Single responsibility policy problem

Question

I'm a bit stuck using the single responsibility policy.

I have a class named Parser, which can be configured to parse input a certain way. For the sake of convenience I will limit the configuration properties to a single property config in the following example:

class Parser {
 boolean config;
 State state;
 AST parse(String input) { ... }
}

I want to reuse the parse instance for multiple parses. The problem is that the state is redundant for the instance itself, however it is required while parsing. Beside the overhead of the state in memory, it doesn't work in multi-threaded environment.

The same goes for a class named Compiler:

class Compiler {
 boolean config;
 State state;
 Code compile(String input) { ... }
}

I was wondering how this is generally solved.

Note: This is maybe a minimal example, but I'm often stuck thinking of a better design.

• Fuhrmanator: I'm not sure that single-responsibility applies here. It's got nothing to do with memory overhead or redundant state.
• Tim: @Fuhrmanator Well, the state is unique per input, not per instance.
• Fuhrmanator: each module should have only one reason to change -- this is in terms of software evolution (changes in source code, not changes in state).
• COME FROM: Usually this is solved keeping the state in a local variable and letting it go after parsing but I guess there's a reason that doesn't work for you. Could you tell us why a local variable won't do?
• Spotted: Who is in charge to create the State ? If a state is bound to an input, why not receiving it as a parameter (in parse or compile) ?

Answer 1

It is OK if your parser and compiler have state, but there is no need to reuse the objects. What I tend to do is offer a static public method Compiler.compile() or Parser.parse() that takes care of instantiating a stateful object, feeds the input to it, retrieves the result, and returns the result while discarding the object. Since static methods are problematic, we can split the stateful parser thing from the public API that provides the parse() method:

public interface Parser {
 public Ast parse(String input);
}
/*internal*/ interface ParserImpl {
 Ast result();
 void doFoo();
 void doBar();
}
class ParserAPI implements Parser {
 @Override
 public Ast parse(String input) {
 ParserImpl p = makeParserImpl(input):
 p.doFoo();
 p.doBar();
 return p.result();
 }
 protected /*virtual*/ ParserImpl makeParserImpl(String input) {
 return new DefaultParserImpl(input, otherDependency);
 }
}
/*internal*/ class DefaultParserImpl implements ParserImpl {
 State state;
 DefaultParserImpl(String input, Dependency otherDependency) {
 ... // initialize state
 }
 @Override
 Ast result() { ... }
 @Override
 void doFoo() { ... }
 @Override
 void doBar() { ... }
}

What is the point of this indirection? Testability. The ParserAPI class uses the Template Method Pattern so that subclasses can swap out the dependency on the DefaultParserImpl, e.g. to use a mock object. While a user of the Parser can keep calling parse again and again on the same object, in fact we are re-creating a new state for each parse, and don't keep the parse state around after parsing is completed. This not only makes the parser and code that depends on the parser easy to test; creating a fresh object for each parse also makes it easier to reason about correctness.

The disadvantage is that we now have one more class in our system, so we do lose a bit simplicity. However, this is offset by the simpler API that is exposed to a parser user.

• Andy: I don't like this approach at all, it leads to tight coupling and violates the open/closed principle.
• amon: @DavidPacker I find that surprising because this design tries to avoid those problems: since the architecture allows all dependencies to be swapped out, you can substitute the DefaultParserImpl with anything you like – which seems to satisfy the OCP as I understand it. Since consumers never get a handle on the stateful parser, they cannot be tightly coupled to it, so the design provides clean isolation. Could you explain your criticism more carefully so that I can improve the design?
• Andy: In order to use a different parser, you will need to change the makeParserImpl method, which is against the closed for modification part of open/closed principle. In your case ParserApi acts as a decorator, it would be much better if it implemented the Parser interface and took a variable of ParserImpl type as a constructor parameter. You get rid of the ugly static method and have as much or more versatility as you had before.
• amon: @DavidPacker In order to use a different parser, you can subclass the ParserAPI and override makeParserImpl(), which can be understood as a template method. It precisely cannot take an instance of ParserImpl as a constructor parameter because that would instantiate the ParserImpl too early and require reuse of the same object. But you're right, in a functional language I'd pass a callback to create the ParserImpl as a parameter, rather than requiring construction to happen in a method.
• Tim: @amon Isn't this close to the factory pattern? I like it, but I still would like to see some other options, therefore I won't accept it (yet).

Answer 2

I once faced a similar problem. While my solution certainly is not optimal, it did the trick.

I used an additional layer between the actual compiling and the concurrently used Compiler-class. This class would be called Compilation in your example.

/*
 This class exists once for multiple (simultaneous) inputs
*/
public class Compiler{
 CompilationParameters compilationParameters;
 public Compiler(CompilationParameters params){
 ...
 }
 public CompilationResult compile(CompilationInput input){ // Of course you can simplify this to a string
 Compilation compilation = new Compilation(compilationParameters, input);
 compilation.compile();
 return compilation.getResult();
 }
}
/*
 This class exists exactly once per input
*/
public class Compilation{
 CompilationState state;
 public Compiler(CompilationParameters params, CompilationInput input){
 ...
 }
 public void compile(){
 ...
 }
 public CompilationResult getResult(){
 ...
 }
}

Now you could argue, that this violates the Open-Closed-Principle. And you'd be right. This solution can be extended for different types of compilations using a factory-pattern for the creation of the Compilation-object. Imagine the following:

public abstract class Compilation{...};
public class CompiletimeOptimizedCompilation extends Compilation{...};
public class RuntimeOptimizedCompilation extends Compilation{...};
public class CompilationFactory{
 public static Compilation createCompilation(CompilationType type, CompilationParameters params, CompilationInput input)
 {
 switch(CompilationType)
 {
 case eCompileTimeOptimized:
 return new CompileTimeOptimizedCompilation(params, input);
 case eRuntimeOptimized:
 return new RuntimeOptimizedCompilation(params, input);
 case ...
 }
 }
}

In this case Compilation might also be an interface instead of an abstract class. To decide that I'd probably need more details about the implementations. And of course the Compiler-class would boil down to:

public class Compiler{
 CompilationParameters compilationParameters;
 public Compiler(CompilationType type, CompilationParameters params){ // This type CAN be part of the parameters
 ...
 }
 public CompilationResult compile(CompilationInput input){ // Of course you can simplify this to a string
 Compilation compilation = CompilationFactory.createCompilation(type, compilationParameters, input);
 compilation.compile();
 return compilation.getResult();
 }
}

Of course the same works for your Parser-class, but I thought it would be a bad example because the noun of "to parse" is "the parse".

If you have any further questions or remarks, don't hesitate asking.

PS: Please excuse any spelling mistakes. I'm not a native.

Answer 3

If your State is needed only by parse() method, why do you keep it in the instance? Why not to create State objects only when they are really needed? You could then pass them around to the methods, called by parse() if needed.

If most methods inside Parser use State, then you should just not reuse the instance. Object construction is usually very cheap if you keep your constructors lightweight. If configuration of the Parser is complex, use factories to construct them. Use Flyweights if configuration process itself is time-consuming.

If you can avoid using single object from different threads - please do so at all costs. The less synchronization problems you have the better, believe me.