Platinum Partner
java,patterns

An (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!

From http://alexruiz.developerblogs.com

{{ tag }}, {{tag}},

{{ parent.title || parent.header.title}}

{{ parent.tldr }}

{{ parent.urlSource.name }}
{{ parent.authors[0].realName || parent.author}}

{{ parent.authors[0].tagline || parent.tagline }}

{{ parent.views }} ViewsClicks
Tweet

{{parent.nComments}}