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.


  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 ?


  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.

  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.

  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.

  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.

  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!

  7. This comment has been removed by the author.

  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.

  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..

  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.

  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...

  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 ?

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