Thursday, November 11, 2010

To reuse or not to reuse, that's the question

It's clear that code duplication is a very bad code smell. As a result you use things like extract method refactoring to keep your code DRY. This brings up the question if this can be taken too far? Should all code duplication be avoided at all times?

Let me illustrate. Assume you have a bit of code like this:
public static void notNull(Object obj) throws IllegalArgumentException {
 if (obj == null) {
  throw new IllegalArgumentException("Argument cannot be null");
 }
}
This method is clearly designed for reuse. It's a small building block of code that can be used thoughout a piece of software to avoid duplicating null-checking code in several places. In this case there are also no real downsides to consider.

Now consider a somewhat higher level piece of (admittedly extremely contrived) code:
public void doSomeProcessing(String str) {
 checkPreconditions(str);
 ...
}

private void checkPreconditions(String str) {
 Check.notNull(str);
 if (!"A".equals(str) && !"B".equals(str)) {
  throw new IllegalArgumentException("Argument can only be A or B");
 }
}
As it stands right now the private checkPreconditions() method is not designed for reuse. If another component in the system happens to require the exact same preconditions, should you try to reuse it? In some situations the answer to this question is clearly: yes! If the new component intrinsically requires the same preconditions, it's natural to try to reuse the method and highlight the fact that the two components have some functional relation. However, in other situations the fact that the two components do the same pre-condition checks might be purely accidental. In other words: there is no functional relation between the two components. In this case refactoring the code to be able to reuse the checkPreconditions() method in both cases comes with a few downsides you should consider:
  • By reusing the method, the refactored code now potentially communicates a relationship between the two components, where there really is none.
  • If the preconditions of the two components are in reality unrelated, it would not be uncommon for the preconditions
    of one of the components to change, while the other component's preconditions remain the same. When making this change in the refactored code, you need to realise you can't just change the common checkPreconditions() method. Instead, a more complex code change will be required.
In my experience DRY code is certainly something to strive for. However, avoiding code duplication at all costs also comes at a cost!

1 comment:

  1. I totally agree.
    Unrelated pieces of code should never create dependencies between them if they are related to functional specifications at least.

    Sometimes you could generalize some functionality to use it in other places. In your example you can write the test with something like:

    Check.StringIn(str, "A", "B");

    public static boolean StringIn(String s, String... options) {
    Check.notNull(s);
    for(String o : options)
    if (s.equals(o)) return true;
    return false;
    }

    Regards,
    Gabriel

    ReplyDelete