Friday, June 27, 2008

When to DRY and when not to DRY

Definitely creating DRY code helps to some extent with maintainability, but it also often makes code harder to read, and tighter, which may hamper maintainability.

These day's I'm often seeing code that's not DRY specifically regarding loops , or loops of loops.

And almost always (I just can't seem to help myself) I'm pulling those code blocks into methods then passing an interface in to do the work. I don't think this neccisarily makes things any clearer, but then I don't seem to be able to help myself from pulling these loops apart.

http://blog.jayfields.com/2007/06/testing-inline-setup.html
Is advocating ignoring DRY for tests, and it can't help but make me wonder when else you shouldn't ignore DRY.

"Another question that always comes up: What about writing DRY code? For me, the value behind DRY has never been typing something once, but that when I want to make a change I only have to make it in one place. Since I'm advocating the duplication in a test file, you still only need to make the change in one place. You may need to do a find and replace instead of changing one helper method, but the other people who are stuck maintaining your tests will be very happy that you did."

for example I took some code that was generating matlab strings that looked like so:

createStringForStuff(List<stuff> stuffs) {
 StringBuffer output = new StringBuffer("StuffName");
 output.append(" = [");
 for (Stuff stuff : stuffs) {
     String text = stuff.isItReallyStuff() ? "1" : "0";
     output.append(" " + text);
 }
 output.append("]");
 return output.toString();
}

createStringForThing(List<thing> things) {
 StringBuffer output = new StringBuffer("ThingName");
 output.append(" = [");
 for (Thing thing : things) {
     String text = thing.isItReallyAThing() ? "2" : "-1";
     output.append(" " + text);
 }
 output.append("]");
 return output.toString();
}

etc.

I pulled the common loop out into a Code Generator Class that took a closure. It ended up okay. But I'm looking at the result. Which looked like:

createStringForStuff(List<stuff> stuffs) {

   MatlabCodeGenerator generator = new MatlabCodeGenerator("StuffName");
   String result = generator.getCode(stuffs, new MatlabClosure()<stuff> {
     @Override
     public String get(Stuff stuff) {
        return stuff.isItReallyStuff() ? "1" : "0";
     }
   });
   return result;
}
So yes it's less code. but reading that doesn't really give me any insight into what the code is doing. Where as before it wasn't DRY, but then again it also wasn't a lot of code

2 comments:

Eric said...

So, I'm trying to less rules-based then in the past, but to say that test code is immune from good principles seems dangerous. DRY is a beautiful thing and test code is still code.

One of the comments on Jay Fields' blog talks about how extracting a method can both increase or decrease complexity depending on usage. I think that is true in the case of DRY as well. And true of abstracting and OO design in general. I would imagine that if DRY is creating problems for your test code then there are more systemic problems to worry about.

Thrilled to see you posting on a blog though. Even though your insights are totally wrong :), it's always a good time trying to think about things that we may have taken for granted and to review basic tenets from time to time.

dpierkowski said...

Proof I should never have a blog. It takes me forever to respond to things...

Sure, it may be DRY, but talk about in-Humane! You took simple little helper methods and split them up in to several different classes. You even put the business logic in an anonymous class. As humans we name things to help us build and maintain mental models of things. Choosing to encapsulate your business logic into something that is inherently unnamed makes it harder for other developers to build the mental model of the code.

Why not something like:


MatlabCode isStuffCode = new MatlabCode(new IsStuffCharDecider(), List<Stuff> stuffs);

MatlabCode isThingCode = new MatlabCode(new IsThingCharDecider(), List<Thing> things);

[Apologies for bad formatting. I can't be bothered to argue with Blogger about how to display that correctly. Cut and paste it in to a text editor.]

 
Web Statistics