When to stop DRYing up the code?

Question

So DRYing up code is supposed to be good thing right? There was a situation in one of the projects I was working on where there were certain models/entities that were more-or-less the same except the context in which they were being used. That is, Every such entity had a title, descriptions, tags, a user_id etc and some other attributes. Hence their CRUD actions in their respective controller looked pretty similar.

My manager argued that its repetition of code and needs to be DRYed up. Hence he came up with CRUD ruby module that when included took care of CRUD actions for the controllers of all these entities. But eventually, Simplicity was compromised. The code lost readability as every "thing" was named "object". Debugging became difficult and the whole point of DRYing up the code was lost.

This was just one case. There are several of them where DRYing up resulted in complex, hard-to-debug code. So the question is, when do we stop DRYing up the code? Because not every time you realize the code has lost simplicity (And often the code author never realizes the simplicity of code is lost). Also, if we have to choose between simplicity and DRYed code, what should choose, if at all there comes a situation where you could get only either of them.

Answer 1

From what I understand, if DRYing up code is causing loss of simplicity, we are doing something terribly wrong. I think, we should be DRYing up code that is repeated and has single responsibility. If the code responsibilities are different and/or the abstraction of entities cannot be named, we are not repeating code. The code pattern might be repeated but its a different code altogether with a responsibilty of its own. If DRYing is resulting into vague code, you are probably trying to DRY up code with different responsibilities that have a similar pattern which is not really a good practice. DRYing should enhance the simplicity, not suppress it.

Answer 2

Readability and Maintainability are the two most important features of good code. Unfortunately, a compromise has to be made sometimes. It's a balance question and not everyone is going to agree.

Myself, I lean towards your point of view as well. I would rather have some apparent repetition if it means the code is easier to understand.

Answer 3

As for the 'debugging' problem, I am in the habit when I create such a 'base class' to include a supplementary field. This field is a simple string which identify the most derived class (and is thus passed from Constructor to Constructor). Then each message is going to print this field + the object id "realtype[id]" and everything is suddenly much easier to debug.

Now on to DRY.

There are two things to DRY:

• building a hierarchy
• using generic code

The first point should now be well understood. A hierarchy of class means a IS-A relationship. If two classes have similar behavior but are otherwise functionally unrelated, then they SHOULD NOT be part of the same hierarchy. It only confuses the poor maintainer and hurts readability.

The second point can be used much more often, especially with scripting languages. For the precedent example, I would argue that instead of having a hierarchy of classes, you could simply define generic methods that would take different classes (modeling different business) and treat them uniformly. This way you avoid repetition (DRY) yet you do not sacrifice readability (imho).

My 2 cts.

Answer 4

If someone every told me--with a straight face--that my code needed DRYing, I would probably take that as a sign that anything else they were going to do was going to be really far-fetched and for-the-sake-of-it.

That having been said, there is also a difference between simplicity in writing code (laziness) and simplicity in the code itself (elegance). I agree, though, that there is a balance. I had this situation myself one particular time (in PHP, but oh how it reminds me of your dilemma):

$checked = ($somevariable) ? "checked=\"checked\"" :"";
echo "<input type="radio" $disabled_checked />";
$checked = ($someothervariable) ? "checked=\"checked\"" :"";
echo "<input type="radio" $checked />";

This isn't even a very good example of what I was dealing with. Essentially, because it's a radio input, both inputs needed a some way of knowing which was to be bubbled in. I knew it had what your boss might call "wetness" issues, so I racked my head trying to come up with some solution that would be graceful and to the point. Finally I showed it to a senior developer and he said "No, it's all in order, it does what it needs to. It's only one extra line."

I felt a relief at being reminded that I was hurting my project more then helping by worrying over this, but at the same time, I'm still disappointed that he was so casual about a fundamental principle (as though it wasn't one of his, though I'm sure it is).

So while I agree, your manager probably was doing something just for the sake of doing it, it is only when we strive to come up with the better methods and approaches that we get better languages like Ruby and Python and cooler libraries like Jquery.

Basically, what if next week you suddenly had 70 things instead of 2? If your boss's objects make that a snap, he was right. If it's the same amount of trouble (in the code or in the execution), he was wrong. But that doesn't mean there isn't a better answer then keeping it simple because it's only a couple of things.

Answer 5

The aim of the DRY principle is to help increase the "quality" of the code.

If the changes aren't improving the quality of the code, it is time to stop. The ability to judge this comes with experience. As requirements change, the most appropriate way to refactor the code also changes, so it's impossible to have everything ideal - at least you need to freeze the requirements first.

Minimising the size of the code should generally not be a consideration in the quality unless you are codegolfing, so don't DRY when the only purpose is to reduce the size of the code.

Complicated tricks can do more harm than good.

Answer 6

A key reason for applying DRY to improve maintainability is to ensure that when a code change is necessary, that change only needs to be made in one place, thus avoiding the risk that it doesn't get changed everywhere that needs it.

But I'm not telling the whole story:

This interview with Dave Thomas has DT saying:

DRY says that every piece of system knowledge should have one authoritative, unambiguous representation.

The first time I saw "DRY" was in The Pragmatic Programmer so I'm inclined to go with Dave on this.

There's another article worth reading here

But DRY is a principle, not a rule: the better we understand the principle, the more able we should be to recognise situations where it should be applied.

(And finally, I think I'd want a little more than "more-or-less the same" before I started "DRY"ing that code: if I could see a clear way in which the two things might diverge in the future then I'd be inclined to leave them alone).

Answer 7

For me, duplicated code is a smell that can have multiple origins:

• Missing variables (introduce variable).
• Missing methods (push expression into a method).
• Feature envy (push behaviour down into the envied class).
• Over-generalization (break up generic class into specific concrete classes).
• Insufficient abstraction (push attributes and behaviour down into new class).

This list is probably incomplete. Consider it a starting point.

When you find duplication, think about what problem it's symptomatic of. Then take a stab at addressing that problem. When you're done, consider the readability of the new code. If it has deteriorated, you may be in one of these positions:

• You misidentified the problem at the root of the duplication (revert, rethink, try again).
• The duplication is a necessary trade-off (revert your change and live with it).
• Your software is necessarily complex (commit your change and live with it).

Consider posting example code along with questions like this, if possible. They provide something concrete to work around. And remember, a lot of this stuff is very subjective.

Answer 8

If you are following REST, then yes, the controllers will be very similar and largely boilerplate. I agree with your manager that it's a problem.

It sounds though like he came up with a suboptimal solution. For a better one, check out Jose Valim's inherited_resources plugin that is being incorporated into Rails 3.