Given an interface for the Strategy pattern like so:

public interface Strategy {
  public Output execute(Input input);
  public Output getLastExecutionOutput();

if I want to store the last execution result of any Strategy implementation is it correct to define an abstract class like this

public abstract class AbstractStrategy implements Strategy {
  protected Output lastOutput;
  public abstract Output execute(Input input);
  public abstract Output getLastExecutionOutput();

so the implementations can inherit this state?

If this is ok then should the abstract class also implement the getLastExecutionOutput method, in which case the field should become private?


IMHO you can do this, and there may be situations where this is the most simple solution (for example, when you are forbidden to change the interface in any way), but to my experience a more SOLID approach is not to make the getLastExecutionOutput part of the strategy interface. That would eliminate the need for the AbstractStrategy class at all.

Instead, look at the place (typically a template method) where execute is called. It is probably a better idea to let the caller store the last output by itself. The whole idea of the strategy pattern is to separate the parts of "different strategies" from the parts which are not different in each strategy. This makes your strategy objects simpler, with a better distribution of responsibilities. The interface in your example assigns each strategy object two mostly unrelated responsibilities (beyond the responsibility for providing the individual strategy code, it has the additional responsibility to cache the last output) and so implies a violation of the SRP.

Assumed you cannot change the interface and must implement the methods above: in this case I would consider to implement this in a slightly different way (I am not a Java guy, so please forgive me any potential syntactical errors):

public abstract class CachingStrategy implements Strategy {
  private Output lastOutput;
  public Output execute(Input input)
       lastOutput = executeInternal(input);
       return lastOutput;

  protected abstract Output executeInternal(Input input);

  public Output getLastExecutionOutput()
       return lastOutput;

Now, when deriving from this class, you have to implement executeInternal instead of execute in each child class. The notable difference is that your child classes now don't have to provide code any more for setting lastOutput correctly, this responsibility is now completely in the CachingStrategy class.

As I wrote above, I think this is only the 2nd best solution. When you have two almost equal solutions, one with use of inheritance and one without, as a rule of thumb, try to use the one without inheritance first. Using inheritance here does not seem to be a big problem at a first glance, but think about what might happen when you get additional requirements "for all strategies" like "firing an event whenever execute was called", "caching not only the last output, but also the last input", and so on. The straightforward solution then seems to implement all these new functionalities into your "common base class", which might end up in a big bloat.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.