Blog

Since I sometimes need to argue with other developers about my understanding of the DRY (Don’t Repeat Yourself) principle I want to share my thoughts about it here.
(So I can give them a link to it and stay DRY, lol)

The DRY principle originates from the book “The Pragmatic Programmer” by Andy Hunt and Dave Thomas. And it’s repeated in a variety of other sources like Kent Beck’s extreme programming…

Basically it is about removing duplicated code by introducing abstractions.

Which is a good idea. Most of the time.
But one thing a lot of programmers don’t really focus on is that abstractions increase complexity.

How often have you seen code like this:

/**
* Returns the value objects, optionally only metadata or throws an exception with the specified Http status code
*/
public List<ValueObject> getDataFromSomewhere(boolean metadataOnly, int httpStatusCode) {
    List<ValueObject> valueObjects;
    try {
        if(metadataOnly) {
            valueObjects = somewhere.loadMetadata();
        } else {
            valueObjects = somewhere.loadFullData();
        }
        return valueObjects;
    } catch(SomeException ex) {
        throw new CustomHttpException(httpStatusCode);
    }
}

(real code may have some more lines and more expressive names, but you get the point right?)

When you encounter code like this someone has clearly reduced duplication by creating a generic method for stuff that is similar, but not exactly the same.

The data returned by this function is needed in 2 variants:
Either only the metadata or the full object. So there is a flag indicating that.
And in error cases different HTTP status codes are needed apparently. Again this is done via the worst possible way: A untyped flag.

What’s the problem with this code?

One point is that it is hard to test.

Code like this may introduce behavior that is not necessary (or even not allowed business-wise). Like HTTP-Status codes that don’t exist at all.

This again could be solved by introducing an enum for HTTP-Status codes. But certainly you don’t need to allow all kinds of HTTP-Status codes. Returning “I am a teapot” in production is fun (been there done that) but not appreciated much by the guys monitoring the system.
As a simple step to solve this you can create an enum with just the allowed HTTP-Status codes.

So lets make some improvements to the code above:

public List<ValueObject> getMetadataFromSomewhere(AllowedHttpStatusCode httpStatusCodeOnError) {
    return getDataFromSomewhere(true, httpStatusCodeOnError.toInteger());
}

public List<ValueObject> getFullDataFromSomewhere(AllowedHttpStatusCode httpStatusCodeOnError) {
    return getDataFromSomewhere(true, httpStatusCodeOnError.toInteger());
}

private List<ValueObject> getDataFromSomewhere(boolean metadataOnly, int httpStatusCode) {
    try {
        return tryToGetDataFromSomewhere(metadataOnly);
    } catch(SomeException ex) {
        throw new CustomHttpException(httpStatusCode);
    }
}

private tryToGetDataFromSomewhere(boolean metadataOnly) throws SomeException {
    List<ValueObject> valueObjects;
    if(metadataOnly) {
        valueObjects = somewhere.loadMetadata();
    } else {
        valueObjects = somewhere.loadFullData();
    }
    return valueObjects;
}

This would me a more “Clean Code” approach to do it. The two different public methods here make it easy to read where it’s used. The API is now cleaner.
But what about removing duplication? This really doesn’t do it. It just encapsulates two different behaviors in one private function.

The easiest thing to do here would be not removing duplication at all and have two separate functions:

public List<ValueObject> getMetadataFromSomewhere(AllowedHttpStatusCode httpStatusCodeOnError) {
    try {
        return somewhere.loadMetadata();
    } catch(SomeException ex) {
        throw new CustomHttpException(httpStatusCodeOnError.toInteger());
    }
}

public List<ValueObject> getFullDataFromSomewhere(AllowedHttpStatusCode httpStatusCodeOnError) {
    try {
        return somewhere.loadFullData();
    } catch(SomeException ex) {
        throw new CustomHttpException(httpStatusCodeOnError.toInteger());
    }  
}

This is shorter and the API is as readable as before. But it does contain a portion of duplication. So what?

If you insist you can still remove it. You can use a functional interface and pass the exception handling into a wrapper method.

Here you go:

Create a functional interface:

@FunctionalInterface
public interface SupplierThrowingSomeException<R> {

  R get() throws SomeException;
}

And a static wrapper method:

public static <R> Supplier<R> wrapSupplier(SupplierThrowingSomeException<R> supplier, AllowedHttpStatusCode httpStatusCode) {
    return () -> {
      try {
        return supplier.get();
      } catch (SomeException exception) {
        throw new CustomHttpException(httpStatusCode.toInteger());
      }
    };

And voila, you can use it like this:

public List<ValueObject> getMetadataFromSomewhere(AllowedHttpStatusCode httpStatusCodeOnError) {
    return wrapSupplier(() -> somewhere.loadMetadata(), httpStatusCodeOnError).get();
}

public List<ValueObject> getFullDataFromSomewhere(AllowedHttpStatusCode httpStatusCodeOnError) {
    return wrapSupplier(() -> somewhere.loadFullData(), httpStatusCodeOnError).get(); 
}

Is this cool? Probably. You decide.
Is it worth it? Probably not.

In fact this is stuff I end up with when I do JavaScript for too long and then return to Java…

It’s way too complex for the “problem” it aims to solve, in my opinion.

You see: “Don’t repeat yourself” is a good advice in general. But KISS (“Keep it simple, stupid”) might be the preferable option of these two, if they happen to collide.

What do you think about this?