DRY Is the Enemy

I see code review comments and changes in code reviews where developers obviously have this mantra of “don’t repeat yourself” that they speak over and over. Time and time again I’ve seen it lead to some of the worst spaghetti code. The kind of code that makes me want to respond with a brick. Code that is reasonably well structured and written by competent individuals yet still makes no damned bit of sense. Ready for an example?

You’ve got a class that is doing the display of a couple inputs with autocomplete. All the previous options come in as arrays. So, someone might write something like this:

let option1Initial = option1Value ? [option1Value] : [];

let option2Initial = option2Value ? [option2Value] : [];

let option1TypeAheadValues = allPreviousOption1Options

    .filter((option, index, array) => 

        array.indexOf(option) === index)

    .concat(option1Initial);

let option2TypeAheadValues = allPreviousOption2Options

    .filter((option, index, array) => 

        array.indexOf(option) === index)

    .concat(option2Initial);

That code is probably fine. If you spend a couple minutes, you’ll figure out that it’s removing duplicates in each option and then tacking on the value that is contained in the control itself. It might be a little weird, but, hey, it works.

Now the time comes that another part of the application needs to display these very same autocomplete fields. It would be the exact same code at this point, so the developer pulls these lines out into a helper that looks like this:

let {option1TypeAheadValues, option2TypeAheadValues} = 

    SetupTypeAheadValues(

        option1Value, 

        allPreviousOption1Options, 

        option2Value, 

        allPreviousOption2Options);

Now you’ve done it. “Look, it’s completely DRY! The repeated code in both files is now in just one place.” Good job hotshot. I’d like to give you a high five. To your face. With a brick.

You’ve dried up the code, but you’re replaced it with something completely meaningless. Yes, it is accurate – it’s setting up the type ahead values. You didn’t lie. But, you’ve managed to conceal anything useful about what it’s doing to those values. Under what circumstances do I actually need to set up these values? Does this function alter the objects I’m passing in? What if I need to do more setup this time? Anyone trying to maintain this code will have to bumble around and wonder why we’re even calling this method. Or, even worse, they need to make a change to that method and wonder about the vast side effects.

I feel some of you screaming – “What if I made this a little more general? I could totally do something like this:”

let option1TypeAheadValues = 

    SetupOptionTypeAhead(option1Value, allPreviousOption1Options);

let option2TypeAheadValues = 

    SetupOptionTypeAhead(option2Value, allPreviousOption2Options);

You could write that. And I’d still want to hit you with a brick. Yeah, the method is a little more general purpose (only slightly), but you are still covering the reason for calling the method. What does ‘setup’ even mean in this context? Is ‘setup’ somehow magical? You know what else is magical? Bricks. In your face.

So, what’s the answer? Think about the business rule you’re trying to apply. You want an array that doesn’t have duplicates. You also want to insert the current value, if it’s defined. So, say that. Write it out.

let option1TypeAheadValues = allPreviousOption1Options

    .removeDuplicates()

    .insertValueIfDefined(option1Value);

let option2TypeAheadValues = allPreviousOption2Options

    .removeDuplicates()

    .insertValueIfDefined(option2Value);

Now we have two helper methods that have a clear purpose. They can be used in all kinds of places. And they clearly describe the business rules you’re trying to satisfy. Future developers will love you for this code.

Yes, it’s six lines instead of two. You’ll have to copy those six lines into another file if you need to use this same functionality again. But, you know what’s not repeated? The logic. We’ve put the logic behind nice descriptive names. No one got hit with a brick.