Friday, September 17, 2010

Too Small to Refactor?

In John MacIntyre's second blog about Clean Code he presented a very simple little payroll calculator, refactored it, and then asked whether it was truly worth refactoring in an ROI sense.  His conclusion was that he would probably not refactor it in a "real" project, but probably would refactor it if it were his own project. His reasoning was that the refactoring was worth doing for artistic reasons, but not for financial reasons.

This argument suggests that Clean Code is about art rather than money.  This is a fundamental flaw in logic.  Clean code has always been about money, and has never been about art. Craftsmen keep their code clean because they know that clean code costs less.  They know that cleaning code has a one-time cost, but that leaving it unclean has a long-term repeating chronic cost that increases with time.  Craftsmen understand that the best way to reduce cost and increase ROI is to keep their code very clean.

Here was the code that John began with (I've translated it from C# to Java for my own sanity.)

If we are going to refactor this, we're going to need some tests.  So the first thing I did was to write enough tests to cover the code. 


The algorithm was a little bit wordy, so I shorted it up a bit and made the two sections of the if statement independent of each other.


Next I got rid of that boolean argument. Boolean arguments are always troublesome beasts. Some poor schmuck is bound to call it with the wrong value, and all the poor people reading this code will wonder whether they should look up the argument to see what it means. Boolean arguments loudly declare that this function does two things instead of one thing.

This had a profound effect on the tests. The tests look almost like they are using two derivatives rather than two instances of the same class. Indeed, we should probably continue pushing the refactoring in that direction. Creating two derivatives is simple enough. First I changed the tests to create instances of the derivatives, and then I wrote the derivaties themselves.

That sets things up nicely. Now I just need to push the calculate method down into the two derivatives.

Nice! Now all I need to do is refactor the two derivatives.

Now that's nice!  Nearly the same number of lines of code as the original,  and so much cleaner! But was it worth it?

Of course it was!  The two business rules have completely decoupled from each other.  They are in totally different files, and know nothing about each other.  If someone adds a new kind of pay calculation, like a SalariedCalculator, none of these files will need to change!  (We call that the Open Closed Principle by the way.)  Think about what we'd have had to do with the old implementation!  Boolean's don't split into three very well.

Yes, this was worth it.  It was worth it because we've all been impeded by bad code before.  We all know that bad code slows down everyone who reads it, every time they read it!  Bad code is like Herpes.  It's the gift that keeps on giving.

21 comments:

  1. Given the original code was already handling two cases of different pay calculations would it not be wise to delay the refactoring to the time when the third type of payment calculation was presented as a requirement ?

    regards,
    Thomas

    ReplyDelete
  2. Thomas, I personally don't think it's better to wait...

    First of all, when you encouter the above untested code it is always better to write a test for it, agreed? Sure you can wait with that, but it prevents the calculator logic from accidental change.

    So, you are writing this test code and thinking about what it does. Why NOT refactor the code?

    If you wait and in the future you or somebody else needs to add that third calculator, are you going to read that bad code again? That would probably take just as long as changing it right now. Why wait if you notice it now (while writing the test)?

    The boy scout rule has proven itself on many occasions.

    ReplyDelete
  3. Besides that, the pay rate calculation algorithm in the final code is considerably easier to follow, thus someone modifying it is far less likely to make a mistake. But the whole point was, no one would have to modify the final result, only extend the base class for whatever next use case may occur.

    ReplyDelete
  4. I would even take it a step further and make calculate a template method in PayCalculator so I can be sure validateHours is always called.

    http://gist.github.com/585100

    ReplyDelete
  5. Depending on the project you may not even know about the existence of untested and dirty code. If you have several novice developers on your team it´s not difficult (at all) to find yourself navigating on untested and poor code.

    When I face situations like that if the code it´s not in the scope of my current activity, I normally create a new technical debt like ticket on the issue tracking (referencing the component with dirty code) so we can prioritize the refactorings based on effort and criticality.
    Otherwise, if I´m fixing a bug or changing something within that code the boy scout rule applies.

    ReplyDelete
  6. Very nice Bob.
    Another great example of good clean code.
    Love how you pulled out the boolean.
    Much cleaner.
    Much simpler.
    Much easier to change.
    Much easier to maintain.

    Keep'em coming!

    ReplyDelete
  7. This comment has been removed by the author.

    ReplyDelete
  8. I'm back and forth on your refactorization, Brian Gamble. In the future extension example given, it seems unlikely that you would want to validate hours on a salaried worker (at least for the purposes of payroll calculation).

    Also, I'm interested in how this might extend for something like a commission pay calculator. Obviously rate and hour wouldn't be enough to represent what would be needed. I'm having trouble seeing how that might evolve.

    ReplyDelete
  9. In reply to royvanrijn. If I was to add tests to every bit of untested code I encounter in the legacy codebases I work with I would not be able to deliver any visible value to the customer for long periods of time. Therefore I only write tests for bits of code I have to change. If the customer allows for 'general quality improvement' I focus the refactoring effort on areas that are likely to change. Not sure if the given example code is likely to change. As always, it depends..

    ReplyDelete
  10. @rmyers - For a salaried worker, you would need a different interface since gross salary and the number of pay periods per year would be needed. For an hourly worker, I was thinking that validateHours was a business requirement and would need to be enforced when calculating pay. I could see it going either way though.

    ReplyDelete
  11. I'm still astonished by "Clean code has always been about money, and has never been about art". I thought we were talking about craft, and the cost-cutting as a subproduct...

    ReplyDelete
  12. "I would even take it a step further and make calculate a template method"

    Why not using interfaces ? I would change PayCalculator into an interface as this is even more clear points out to a concept.

    Second I would change the validateHours-method to a static method since it seams to be stateless (and put it into a utility class). Makes it even more reuseable. This step also shows that you have to think about that magic number 80. Why 80 ? Why 40 ? Is it half of 80 or just an accident ?

    ReplyDelete
  13. افضل شركة مكافحة حشرات المتخصص فى كافة انواع القضاء على الحشرات التى نقدمة الان من شركة ابادة حشرات كما اننا نقدم افضل المواد للقضاء على العديد من الحشرات الان

    ReplyDelete
  14. تمتع الان معنا مع افضل الخدمات التى نقدمة الان وعلى اعلى مستوى ممكن من شركة ابادة حشرات التى لا احد يقدمة الان , التى نقدمىة الان من اهم الشركات

    ReplyDelete
  15. سارع الان معنا بالتواصل مع افضل المميزات الرائعة التى نقدمة الان على اعلى مستوى ممكن الان من تجهيزات سوبر ماركت فى مصر , وباقل الاسعار الممكن فى كافة المحافظات التى نقدمة الان فى مصر من تجهيزات محلات التى نقدمة الان فى مصر وعلى اعلى مستوى ممكن من تجهيزات مطاعم , وبالق الاسعار الممكن الان

    ReplyDelete
  16. خصومات الان , بوقال الاسعار الممكن فى تركيا الان من مركز زراعة شعر لاننا نعرف ان العديد من السيدات توجة العديد من المشاكل فى الشعر لذلك نقدم الان زراعة الشعر وباقل التكاليف الممكن

    ReplyDelete

  17. شركه عزل فوم بالقطيف

    شركه عزل فوم بالاحساء عزيزى العميل اهلا ومرحبا بك فى موقع مؤسسة الحرمــين للمقاولات العامة والعوازل
    شركه عزل فوم بالاحساء و الرياض
    الموقع الرائد فى عالم الخدمات المنزليه والاول بالمملكه العربيه السعوديه لما يتمتع به من خدمات مميزه ، فالبرغم من اننا مؤسسه ربحيه الا ان مزاولة نشاطتنا

    شركه عزل فوم بجدة

    شركه عزل فوم بمكة

    شركه عزل فوم بالرياض كلها مرتبط على نحو وثيق
    بتلبية طلبات وحاجات عملائنا ولتحقيق ذلك الهدف نقدم لك كافة الخدمات الشامله بالالتزام الصارم وبأرقى المعايير المهنيه المتطوره
    فلدينا خبره طويله فى مجال مكافحة الحشرات والكشف عن التسربات وتسليك المجارى وعزل الاسطح وترميم وصيانه المنازل وتخزينه بكفاءة منقطعة النظير ، لا تتردد واتصل
    بموقع مؤسسة الحرمــين فخدماتنا ليس لها بديل واسعارنا ليس لها مثيل ،ولدينا فريق عمل يتصل مع العملاء على جسور الثقه
    شركه تنظيف منازل بالجبيل
    والصدق والامانه فى العمل ، وهدفنا هو ارضاؤك وراحتك ، لا تقلق ونحن معك

    شركه عزل فوم بالجبيل
    لا تجهد نفسك ونحن تحت امرك ورهن اشارتك .
    أبرز خدمات مؤسسة الحرمــين للمقاولات العامة بالدمام والرياض

    شركه عزل فوم بالدمام

    شركه كشف تسربات المياه بالاحساء



    شركه عزل اسطح بالاحساء


    شركه عزل فوم بالقطيف

    شركه عزل فوم بالاحساء

    ReplyDelete
  18. minuman penyegar seperti bentrap original cod di tambun utara sekarang ini banyak digemari oleh pria untuk memelihara stamina agar tetap perkasa saat berhubungan badan. produk bentrap asli cod karawang ini banyak diminati karena sudah memiliki sertifikat halal yang keabsahan bisa anda yakini 100% dapat memberi manfaat bagi pria yang ingin mendapatkan klimaks saat berhubungan badan. bentrap asli cod bandung dikemas dalam bentuk serbuk dengan aturan pakai seperti minuman yang mudah dikonsumsi bagi siapapun tanpa pengecualian sekalipun bagi pria yang menderita penyakit seperti diabetes. bentrap asli cod jakarta timur dapat diterima oleh semua kalangan dan hasilnya juga sangat memuaskan bagi setiap pasangan yang ingin menjaga keromantisan dalam bercinta agar selalu optimal setiap libido memuncak. baca seterusnya . wanita bisa merasakan kebosanan kalau pasangannya tidak dapat memberikan kepuasan seksual saat diranjang, untuk itu anda sebagai pria harus mencari solusi dengan cara mengonsumsi permen soloco cod-di-tembalang sebagai obat untuk meningkatkan libdo serta membuat ereksi lebih keras dan tahan lama. klik seterusnya .

    ReplyDelete

  19. نقل عفش داخل جدة نقل عفش داخل جدة
    دينا نقل عفش جدة دينا نقل عفش جدة
    افضل نقل عفش من جدة الى الرياض افضل نقل عفش من جدة الى الرياض
    نقل عفش من جدة الى دبي شحن عفش من جدة الى الامارات

    ReplyDelete
  20. Faisalabad is one of the biggest cities in Pakistan and the hub of the textile industry. It is widely acknowledged as the Manchester of Pakistan due to its large industrial role. The quality of the fabrics produced in this city has no parallel. In fact, the fabric is something of a specialty of Faisalabad. Many people from all over the country flock to this city for a spot of cloth shopping. We aim to provide you all of the best of Faisalabad at our store. pakistani designer suits wholesale uk , pakistani designer suits wholesale uk

    ReplyDelete