Don’t write code that is too DRY

DRY, the acronym for Don’t Repeat Yourself, is a principle that encourages developers to write everything once and avoid redundancy. It’s great, it makes a whole lot of sense, but it can easily have a detrimental effect if taken too literally. I’ve often advised caution when abstracting code out of components, and I’m particularly wary when code is moved into files called “settings”, “util”, “common” or some sort of variation of a generic filename.

Imagine the following use case:

ComponentA uses functionA. ComponentB also needs functionA, so functionA is moved to a “util” file, and ComponentB now uses functionA. A new requirement for ComponentB requires functionA to be modified. Because the change would cause an unwanted change in ComponentA, functionA is duplicated to functionB which is modified to accommodate ComponentB. ComponentA uses functionA and ComponentB uses functionB but the functions remain in the “util” module! Each function is now used only once, yet are not located with the only code that actually needs them. The proper fix would have been to move functionA back to ComponentA and functionB back to ComponentB. These functions should have been ‘unabstracted’.

Here is another situation:

The source code of a large application is organized in a monorepo, and each team contributes to a different module. Team A contributes to @repo/featureA, team B contributes to @repo/featureB, and so on. Developers on each team, very diligently, follow the DRY principle to the letter, and decide to federate common pieces of code in a common folder. Constants go in a constants.js file, settings in a settings.js file (assuming there is a difference and it is understood), and so on…

Soon, the source code looks like this:

@featureA/src/common
                   |-util.js
                   |-settings.js
                   |-constants.js
@featureB/src/common
                   |-util.js
                   |-settings.js
                   |-constants.js

How is this DRY? If as a new developer I start working on @featureC, do I create another util/settings/constants file or (try to) use one from @featureA or @featureB? Is the documentation up to date or do I need to read the whole code? Is the code covered by tests in a way that I’m confident that any change breaking my code will be noticed? If I’m a developer on featureA, what’s my guarantee my common code will not be modified by someone else?

I’ve seen developers take this to an extreme, where any constant variable is automatically moved to a “constants” file even if used once, and code like this starts to creep in:

// !!! multiple constants have the same value !!!
// This is not DRY
const MY_CONST_1 = "some value";
const MY_CONST_2 = "some value";
const MY_CONST_3 = "some value";
// !!! constants used only once !!!
// This extra layer of abstraction is not needed
const MY_CONST_COMPONENT_A = "a value only used in A";
const MY_CONST_COMPONENT_B = "a value only used in B";
const MY_CONST_COMPONENT_C = "a value only used in C";

To confirm this observation, I looked for files containing “constants”, “util” or “settings” in a large codebase, and I found 671 files with a combination of constants, util and setting in their name!!!!

That’s 671 files of code intended to be DRY but is very likely not. I decided to take it a bit further and picked a random variable name, and found it was defined 47 times. Yes the codebase is very large and getting very old, but I will be hard to convince that moving code that is relevant to a component or a module into a hodgepodge of “common” code is a good practice. It might sometimes be a good thing but doing it systematically is not necessarily a good thing.

I want to point out and thank Kent Dodds and Sandi Metz for writing about this, and coining the term AHA: Avoid Hasty Abstraction.

In conclusion, I’d like to present the following code smells:

• A unit of code (class, variable, function, static method) used only once that is not in the same unit of code as its sole caller.
• A unit of code that is obviously business related in a generic unit of code: e.g. a isShoppingCartEmpty function in a util.js file. Why isn’t this function in the ShoppingCart class?
• A function that is just a big if/else where the conditional tests the presence of an argument:

function doSomething(arg1) {
  if(!arg1) { // <-- this can indicate that the code was copy/pasted
    // do something 
  } else {
    // do almost the same thing, but with arg1
  }
}

• A one line function with a generic name that handles business logic

// misleading name
const isArrayEmpty = shoppingCart => shoppingCart.items.some(!tax);

If you can think of other signs of hasty abstraction and examples of code that is too DRY, please let me know in the comments. Other constructive feedback is also much appreciated, thanks for reading!