Refactoring DesignForExtension

TL;DR: Don’t use final methods; use the strategy pattern instead.

CheckStyle contains a somewhat controversial rule called DesignForExtension that forces all non-final1 types to conform to some rather restrictive rules. Specifically, non-private, non-static methods of classes that can be subclassed must

  • be abstract or
  • be final or
  • have an empty implementation.

You can read the rationale behind this on the original page; I won’t risk subverting it by summarising here. Many good examples of inheritance don’t look like this, though: for instance, the abstract collection types in the JDK, such as AbstractList, certainly have some abstract methods, but deliberately leave the others non-final so they can be replaced with a more efficient implementation if desired.

But is declaring methods final ever a good pattern to follow? I’m going to argue it’s not, and to do that, I’m first going to tell you how to refactor them away.

The example

To demonstrate how to refactor away final methods, I’d like to take an example from CheckStyle itself: AbstractFileSetCheck. Unlike the strawman included in the DesignForExtension documentation, this is solving a real problem in a real codebase. However, it’s also very big, so let’s simplify it while retaining the core elements:

public abstract class AbstractFileSetCheck {
  private final Messages messageCollector = new Messages();
  private String[] fileExtensions = CommonUtils.EMPTY_STRING_ARRAY;

  protected abstract void processFiltered(File file, List<String> lines);

  public void init() {
    // Override if necessary in subclasses
  }

  public final SortedSet<Message> process(File file, List<String> lines) {
    messageCollector.reset();
    // Process only what interested in
    if (CommonUtils.matchesFileExtension(file, fileExtensions)) {
      processFiltered(file, lines);
    }
    return messageCollector.getMessages();
  }

  public final void setFileExtensions(String... extensions) {
    fileExtensions = Arrays.copyOf(extensions, extensions.length);
  }

  protected final Messages getMessageCollector() {
    return messageCollector;
  }
}

And here’s a simplified subclass:

public class FileLengthCheck extends AbstractFileSetCheck {

  public static final String MSG_KEY = "maxLen.file";
  private static final int MAX_LINES = 80;

  @Override
  protected void processFiltered(File file, List<String> lines) {
    if (lines.size() > MAX_LINES) {
      getMessageCollector().log(1, MSG_KEY, lines.size(), MAX_LINES);
    }
  }
}

There are many kinds of method here: public final, for external callers; protected final, for subclasses to call; protected abstract, for subclasses to implement; and public but empty, for subclasses to optionally implement. How should we go about refactoring it?

Composition over inheritance

Composition over inheritance is one of the most famous aphorisms in object-oriented programming, coming as it does in the first chapter of Design Patterns. In our example, FileLengthCheck inherits from AbstractFileSetCheck; how do we go about using composition instead?

There are two ways we could compose this; FileLengthCheck could hold a reference to AbstractFileSetCheck, or AbstractFileSetCheck could hold a reference to FileLengthCheck. If we do the former, though, we have no way of enforcing the behaviour of the public final method process, so (perhaps surprisingly) we will always need to refactor the DesignForExtension pattern so the base class holds a reference to the (former) subclass.

public class CompositeFileSetCheck {  // no longer Abstract!
  private final FilteredCheck filteredCheck;
  ...

The two classes are tightly coupled in the inheritance model, with AbstractFileSetCheck calling into FileLengthCheck via processFiltered, and FileLengthCheck calling into AbstractFileSetCheck via getMessageCollector. We don’t want tight coupling in general, so let’s invert control and pass the message collector into processFiltered:

public interface FilteredCheck {
  default void init() {}
  void processFiltered(Messages messageCollector, File file, List<String> lines);
}

Since there are no longer any final methods on this type, we have made it an interface. (If we were stuck in Java 7, we’d have to use an abstract type, since we can’t give init a default implementation.)

public class FileLengthCheck implements FilteredCheck {

  public static final String MSG_KEY = "maxLen.file";
  private static final int MAX_LINES = 80;

  @Override
  public void processFiltered(
      Messages messageCollector, File file, List<String> lines) {
    if (lines.size() > MAX_LINES) {
      messageCollector.log(1, MSG_KEY, lines.size(), MAX_LINES);
    }
  }
}

FileLengthCheck hasn’t changed much at all. What about AbstractFileSetCheck, now CompositeFileSetCheck?

public class CompositeFileSetCheck {
  private String[] fileExtensions = CommonUtils.EMPTY_STRING_ARRAY;
  private final FilteredCheck filteredCheck;

  public CompositeFileSetCheck(FilteredCheck filteredCheck) {
    this.filteredCheck = filteredCheck;
  }

  public void init() {
    filteredCheck.init();
  }

  public SortedSet<Message> process(File file, List<String> lines) {
    Messages messageCollector = new Messages();
    // Process only what interested in
    if (CommonUtils.matchesFileExtension(file, fileExtensions)) {
      filteredCheck.processFiltered(messageCollector, file, lines);
    }
    return messageCollector.getMessages();
  }

  public void setFileExtensions(String... extensions) {
    fileExtensions = Arrays.copyOf(extensions, extensions.length);
  }
}

No more protected methods, no more final declarations… that’s pretty good, actually.

Strategy pattern

You may have recognised this refactored relationship as the strategy pattern, also found in Design Patterns. In fact, that’s the only thing final methods let you build, and in an overly coupled way at that. But, aside from epithets like ‘coupled’, why is it better to implement the strategy pattern with composition rather than inheritance? Firstly, it improves readability. I hope you’ll agree its easier to understand the FilteredCheck interface than the AbstractFileSetCheck class. Understanding the FileLengthCheck class, or creating a new check, requires understanding its supertype, so our refactoring has made it easier to understand the (presumably many) check types that exist. And CompositeFileSetCheck is no harder to understand than before: a net win.

Secondly, it shrinks our API. CompositeFileSetCheck can be package-scoped instead of public, as the shorter and simpler FilteredCheck interface is all clients need to implement.

Thirdly, it enables further refactoring. In this case, the FilteredCheck strategy interface is cohesive, but what if it had contained several unrelated methods, which tend to vary independently? Simple: break the strategy into pieces. Inheritance prevents us from doing such a basic refactoring.

Finally, it simplifies testing. To test AbstractFileSetCheck, we need to create a dummy subclass; to test CompositeFileSetCheck, we simply inject a mock. To test the old FileLengthCheck’s processFiltered method, we need to instead call the process method on its supertype, as it modifies internal state; to test the new one, we can call processFiltered directly. To test a consumer of AbstractFileSetCheck instances, we need to extract a FileSetCheck interface, since final methods break Mockito; to test a consumer of CompositeFileSetCheck instances, we can simply mock some up.

Conclusion

I hope this has gone some way to convincing you that DesignForExtension is a bad rule, made with good intent. Further, I hope you now have the tools to do better, by using composition rather than inheritance, and more specifically by identifying and employing the strategy pattern instead of final methods.


1 Well, extensible, but together with the FinalClass rule it ends up the same.

Advertisements

2 comments

  1. Roman Ivanov · April 16, 2016

    Thanks a lot for your post, you gave me a stimulus to review this Check implementation and documentation and create an issue to finish its implementation – https://github.com/checkstyle/checkstyle/issues/3102 .

    Due to problems in our documentation (we will fix this for sure) you misread rationale of this Check.
    Check is not about prohibiting overriding method.
    Check idea to help user who do development of libraries(api, interfaces, abstract classes, …) to implement override-able methods correctly.
    This Check should not be used in application projects where nobody really care about visibility of methods.
    It is mainly for library projects where each public override-able method will result “contract” with user, immature implementations will make users hate a library design.

    Main rationale is “The best solution to this problem is to aprohibit subclassing in classes that are not designed and documented to be safely subclassed. ” (from book, and out web doc http://checkstyle.sourceforge.net/config_design.html#DesignForExtension)

    There is no problems with public methods, but user need to make it clear (javadoc, usage of override-able methods inside).
    Unfortunately current implementation of this Check is too severe and incomplete (see open issue).

    PS: everyone is welcome to fix this DesignForExtension Check 🙂 to make it complete and reasonable.

    Like

    • Chris · April 18, 2016

      Thanks for your reply, Roman. I’ve been meaning to clarify that the target of this post is more final methods than DFE per se, which I’ve now done. I look forward to a less severe version of DFE that enforces good documentation!

      Like

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s