When is your code DRY enough?

This is an archive of blog post I wrote during my third venture (PullReview).

You're aware that duplication is a rampant disease: more code, more fragile, less maintainable, less readable. You even use one or another tool and make your code reviewed to find most of them.

When facing some duplicate code, you're not always feeling comfortable to dry it up. You're not even sure you'll keep - as is - the code you've just wrote. By experience, you don't want to spend a whole day to end, maybe, with an abstract solution end to reason about.

Should you avoid similar create actions in different Rails controllers? Should you use metaprogramming and avoid long similar case expressions? What about recurring pattern in your Rails routes? Or multiple repetitions in your specs? When should I stop?

You're right to wonder if it's right or not. Reducing duplication is not only time consuming but it could make your code less readable. Eventually, you want to make your code more maintainable, not less.

DRY is not about identical chunks of bits or characters in different places. DRY aims at not repeating domain concepts. If not, there is a risk that some copy of a domain concept won't be maintained properly. For instance, when implementing invoicing, you want the calculation of VAT implemented in only one place. By doing so, if it changes, there is only one place to change.

When removing duplications that doesn't belong to the same domain concept, it's likely that you'll come with a wrong abstraction. If one of the factored domain changes, it will affect the abstraction and every dependencies that has then to be handled as exceptions. Moreover, the code will be hard to understand and to debug, especially if you use metaprogramming.

In order to know when you should dry your code, you could follow the rule of thumb Three Strikes And You Refactor. I personally apply a more specific approach:

  1. when implementing a feature, code without worrying about duplication (in a dedicated branch)
  2. when you've reached a stable implementation, before merging it, check for duplication (thanks to you, code review, a tool)
  3. for each found duplication
    • don't remove it if
      • you don't - yet - understand them as duplicated concepts
      • explicit code is more important as in tests
      • the solution is complex and hard to understand
    • otherwise remove it

In any case, if you find a recurring pattern in your code more than three times, it should particularly gains your attention.

If you feel uncertain, it's better to let the duplication for the moment. Then, if possible, discuss it during the code review. The reviewer will maybe come with another perspective helping you to come with a good abstraction.

If you have any comment, question, or feedback, please share them with me.

Atom feed icon Subscribe to the blog!