A (overlooked?) use case for the Strategy pattern


Composition over inheritance. It seems that we all agree and understand this valuable principle. I’ve been reading a good amount of code lately from some open source projects (including mine,) and I think we are still missing at least one use case where to apply it.

This is what I’ve seen: class A has method x. Just after method x is done doing its thing and before it exits, it calls a (sometimes empty) protected method y meant to be overridden to handle some result or side effect of x (is there a name for this anti-pattern?)

Here is a concrete (and oversimplified) example, taken from FEST:

/**
 * Understands a <code>{@link SecurityManager}</code> that does not allow an application 
 * under test to terminate the current JVM. Adapted from Abbot's own 
 * {@code SecurityManager}.
 */
public class NoExitSecurityManager extends SecurityManager {
  @Override public final void checkExit(int status) {
    if (!exitInvoked()) return;
    handleExitRequest(status);
    throw new ExitException(concat(
      "Application tried to terminate current JVM with status ", status));
  }
 
  /**
   * Implement this method to do any context-specific cleanup. This hook is provided since 
   * it may not always be possible to catch the <code>{@link ExitException}</code> 
   * explicitly (like when it's caught by someone else, or thrown from the event dispatch 
   * thread).
   * @param status the status the exit status.
   */
  protected void handleExitRequest(int status) {}
}

At first glance this code looks OK. Subclasses can do whatever they want when “exit” is called, and since the method checkExit is final, there is no way that subclasses can accidentally (or intentionally) change the intended behavior of the super class.

There are, IMHO, some issues with this approach:

Let's take a look at a simple (silly?) subclass of NoExitSecurityManager that writes a message to a file when exit is called:

public class WriteToFileNoExitSecurityManager extends NoExitSecurityManager {
  // The class FileWriter only exists for the purposes of this example
  private final FileWriter fileWriter = new FileWriter("${temp}/log.txt");  
 
  @Override protected void handleExitRequest(int status) {
     fileWriter.write(concat("Exit called with status ", status));
  }
}

This subclass is actually doing too much: it is preventing an application from exiting (inherited behavior) and it is also handling the “exit request.”

WriteToFileNoExitSecurityManager is not really a NoExitSecurityManager, but an “exit request handler.” Inheritance in this case is, IMHO, unnecessary: the implemented functionality (not the inherited one) in WriteToFileNoExitSecurityManager is self-contained, and it does not depend on the internal state (or identity) of the superclass. In addition, since the method checkExit is final, there can only be one implementation. On the other hand, there can be different implementations of handleExitRequest, each of them forced to be a subclass of NoExitSecurityManager.

Testing is also more complicated in this scenario. To test NoExitSecurityManager, we would need to subclass it (more unnecessary inheritance) and set some flag in handleExitRequest to verify it is called (we could also create a mock, but at the end it’s all the same):

public class NoExitSecurityManagerTest {
  private TestNoExitSecurityManager securityManager;
 
  @Before public void setUp() {
    securityManager = new TestNoExitSecurityManager();
  }
 
  @Test public void should_prevent_application_from_exiting_and_handle_exit_request() {
    // test that an application trying to exit is effectively stopped
    ...
    // here we verify that the call to 'handleExitRequest' was made
    assertThat(securityManager.handleExitRequestCalled).isTrue();
  }
 
  private static class TestNoExitSecurityManager extends NoExitSecurityManager {
    boolean handleExitRequestCalled;   
 
    @Override protected void handleExitRequest(int status) {
      handleExitRequestCalled = true;
    }
  }
}

Enter the Strategy pattern

We can clean up our example by using the strategy pattern.

First, we introduce the interface ExitRequestHandler. It only has one responsibility: to handle “exit requests.”

public interface ExitRequestHandler {
  void handleExitRequest(int status);
}

Then, we can replace the call to the method handleExitRequest with a call to an instance of ExitRequestHandler:

/**
 * Understands a <code>{@link SecurityManager}</code> that does not allow an application 
 * under test to terminate the current JVM. Adapted from Abbot's own 
 * {@code SecurityManager}.
 */
public final class NoExitSecurityManager extends SecurityManager {
  private final ExitRequestHandler exitRequestHandler;
 
  public NoExitSecurityManager(ExitRequestHandler exitRequestHandler) {
    this.exitRequestHandler = exitRequestHandler;
  }
 
  @Override public void checkExit(int status) {
    if (!exitInvoked()) return;
    exitRequestHandler.handleExitRequest(status);
    throw new ExitException(concat(
      "Application tried to terminate current JVM with status ", status));
  }
}

Finally, we can rewrite WriteToFileNoExitSecurityManager as a WriteToFileExitRequestHandler:

public class WriteToFileExitRequestHandler implements ExitRequestHandler {
  // The class FileWriter only exists for the purposes of this example
  private final FileWriter fileWriter = new FileWriter("${temp}/log.txt");  
 
  @Override public void handleExitRequest(int status) {
     fileWriter.write(concat("Exit called with status ", status));
  }
}

It looks like I just moved code around and actually created even more code! True, we have more code, but better-quality code:

  • each class has a single, well-defined responsibility
  • there is no tight coupling between the class using the strategy interface and a specific strategy implementation
  • implementing a small interface is easier than extending a class (no need to worry about the state of the superclass in a particular context)
  • testing is simpler

Here is the updated test:

public class NoExitSecurityManagerTest {
  private ExitSecurityManager securityManager;
  private TestExitRequestHandler requestHandler;
 
  @Before public void setUp() {
    requestHandler = new ExitRequestHandler();
    securityManager = new ExitSecurityManager(requestHandler);
  }
 
  @Test public void should_prevent_application_from_exiting_and_handle_exit_request() {
    // test that an application trying to exit is effectively stopped
    ...
    // here we verify that the call to 'handleExitRequest' was made
    assertThat(requestHandler.handleExitRequestCalled).isTrue();
  }
 
  private static class TestExitRequestHandler implements ExitRequestHandler {
    boolean handleExitRequestCalled;   
 
    @Override public void handleExitRequest(int status) {
      handleExitRequestCalled = true;
    }
  }
}

I had a hard time writing this blog post because the points I just made seemed to be too obvious and well-understood by now. But, by reading code (both mine and written by others) I got the impression that although we understand the “composition over inheritance” concept, it is still not easy to apply it in practice.

Am I being too picky? I hope not (although I admit this issue has been annoying me for some time, I finally had the chance to complain.) IHMO, every detail counts when writing clean code :)

Feedback is always welcome!

Update: This post has been published at Javalobby.

(Image taken from etringita’s flickr stream under the creative commons license)

, ,

  1. #1 by Vladimir on January 29, 2010 - 2:35 am

    Nice article.

  2. #2 by James Arrington on January 29, 2010 - 7:24 am

    I think the “anti” pattern you are referring to in the original code is call the template pattern. Spring uses it all over the place. I invoke the usual statement of the template pattern has its place.

  3. #3 by Christian Posta on January 29, 2010 - 9:14 am

    No, you’re not being to picky in my opinion. I think you you’re correct in seeing two different responsibilities (preventing exit and handling an exit-request event) and separating the implementations into two classes.

    I don’t think class A calling method y just before finishing x (using your example) is an anti-pattern. It could be a very valid implementation of the Template pattern. However, when you have two distinct responsibilities like you’ve identified, then the responsibilities should be split like you did.

    Good post!!

  4. #4 by Christian Posta on January 29, 2010 - 9:16 am

    Edit of previous post. Bad grammar lol!

    No, you’re not being *too* picky in my opinion. I think you’re correct in seeing two different responsibilities (preventing exit and handling an exit-request event) and separating the implementations into two classes.

    I don’t think class A calling method y just before finishing x (using your example) is an anti-pattern. It could be a very valid implementation of the Template pattern. However, when you have two distinct responsibilities like you’ve identified, then the responsibilities should be split like you did.

    Good post!!

  5. #5 by Sai Venkat on February 11, 2010 - 9:52 pm

    Isn’t the first example with inheritance an example of template patter? The classic template vs strategy pattern :)

    Good points to take away.

    Cheers.
    Sai Venkat

  6. #6 by Abhishek on April 19, 2010 - 3:09 am

    Excellent Article.
    I think the first example shows that you are providing a hook in a template method pattern and the re factoring is excellent, but if most of your sub classes are not going to use that method , i would suggest that as a template method pattern, otherwise strategy pattern should be applied …it all depends on the requirements and pros and cons related to each of the pattern.

(will not be published)