In creating a "registry", which is worse: using reflection or violating open/closed principle?

Question

In my current software engineering course, my team is working on a library management system that is essentially a command-line/REPL environment with a half dozen commands, e.g. checkout, search, etc. We've elected to use the command pattern for this, so each command will be represented as a class. Now, there's the question of how we create new command objects when the user enters a string.

1. Use conditionals:

   switch(input) {
       case "checkout":
           return new CheckoutCommand(args);
           break;
       // etc.
   

2. Create a HashMap mapping strings to constructors:

   interface CommandCreator {
       abstract public Command createCommand(String[] args);
   }
   

   Map<String, CommandCreator> commandMap = new HashMap<>();
   commandMap.put("checkout", CheckoutCommand::new);
   commandMap.put("search", SearchCommand::new);
   // etc.
   

   return commandMap.get(input).createCommand(args);
   

3. Use reflection:

   Class.forName("library.commands." + input).getConstructor(args.getClass()).newInstance(args);
   

   The command classes would then have the same names as the strings that invoke them, i.e.

   class checkout implements Command {
       // class body
   }
   

4. Something else.

We agreed that #1 is not the best solution. Some of us think that #2 should not be used, because it violates the Open/Closed Principle; when we add more commands in the future, we would have to modify the class that populates the command HashMap. Some of use think #3 should not be used, because reflection should only be used as a last resort, and it seems too "hacky". However, none of us have any better ideas.

Should we use #2 or #3, or is there a better solution?

Answer 1

The problem with the Open/Closed Principle is that its name implies that it is some overarching guide that must always be followed or else bad things happen. This isn't true.

The OCP is a guideline: useful in some situations, and somewhere between a distraction and outright counterproductive in others. It's very useful in library code that is likely to be reused in multiple places. It's handy at the edges of modules, allowing functionality to be implemented in the correct place without needing coordination between module authors. It has no place at all in the top level code of an application.

It simply isn't realistic to expect to write an entire application and then change its behaviour without changing a single existing module. At some level, you need to coordinate how the new functions integrate with the existing. Typically, this is done using an object graph that is configured at the very highest level of the application.

In your case, that most likely would mean option 2 (although option 1 isn't out of the question, in my opinion, and has the advantage of a direct simplicity that could make it attractive in some circumstances).

Answer 2

Command cmd = commandFactory.create(input, args);

Now which implementation did I use?

Can't tell? That's right, because it doesn't matter. At least, it doesn't matter so long as you have an abstraction that is responsible for creating the command. Hide that logic behind this interface and it simply doesn't matter which implementation you choose. Pick one that works and if you find something to be painful about it, change the guts of that factory latter. Your team has spent far more time bikeshedding about this than is worthwhile.

Answer 3

Use a static factory method.

You're going to have to modify the source code anyways if you introduce new commands so I don't see the issue with the Open/Closed principle; it's similar to #1, but doing it with a static method in the base class is more extensible than #1.

You'd end up with something like this

public abstract class Command
{
    public static Command getCommand( String cmd, String[] args )
    {
        switch( cmd )
        {
            case "checkout":
                return new CheckoutCommand( args );
        }
    }
}

class CheckoutCommand extends Command
{
}

Command.getCommand( "checkout", [] );

Answer 4

I think #4 something else is appropriate here.

I would design this system to use a configuration file that maps command name to class name.

The config could be a JSON file for example:

{
  "commands": [
    {
      "name": "Checkout",
      "longCommand": "checkout",
      "shortCommand": "chk",
      "className": "library.commands.CheckoutCommandFactory",
      "documentation": "Checks-out a book from the library"
    },
    ...
  ]
}

Here is my rationale:

You're right about #1 and #2 being poor design.

Reflection is not a good choice here because you're trying to map user input to class name. You definitely shouldn't name your class the same as the user's command because class names shouldn't be coupled to user interface and you shouldn't subvert class naming conventions. So that would leave string manipulation to try to form the classname from the user input (capitalize the first character and append "Command"), but that would be complicated by multi-word commands. In addition if you decide to change a command you will have to refactor class names. This technique doesn't allow for command aliases, and it makes enumerating all the commands more difficult. What happens if somebody wants to add a command to your program by adding their jar on the classpath--they would have to know and follow the complex naming convention. Using a rigid reflection system is complex and not maintainable.

Using a configuration file will allow you to map commands to classes easily, maintainably, and it supports many use cases and features that you'll likely want.

Note: this solution may add some complexity to your project, but a good compromise if you don't want to add file parsing to your project is to define a class to represent the schema of the file:

public class CommandDefinition {
    private final String name;
    private final String longCommand;
    private final String shortCommand;
    private final String className;
    private final String documentation;
}

You can then create an in-memory collection of these objects to avoid or postpone the file parsing work (essentially falling back to the same advnantages or disadvantages of #2)

Answer 5

I would not use #1, conditionals (switch): each time you have any changes to need to modify the conditional routine and recompile the process that parses commands and executes them.

I would use a combination of Strategy and Builder to handle the construction of each command: each command may have its own contruction process encapsulated in an independent class: the process that executes the commands has the same interface to handle each command (this is what Strategy brings), but each command will have its own construction process (this is what Builder brings).

The use of HashMap or Dictionary is fine.

I would have each Strategy/Builder in an independent component, so that a factory method reads from a config file what component relates to which hash/command, and creates the HashMap or Dictionary at start-up, before the execution of commands start.

Kind regards, GEN

Answer 6

Your concerns about the open closed principle in your suggestions are not correct. The OCP does not mean you may not change any implementation. It means you only have to change the classes where the semantics of the new requirements are adressed.

So #2 of your suggestions is the best solution for a registry.

But there is another possibility: Inversion of control!

What you really want is to introducue a new "Command" without changing something by YOURSELF. So you may use a framework that recognizes new implementations and provide them with a dependency injection mechanism.

Spring example:

public interface CommandCreator { ... }


@Component
public class CommandCreator1 implements CommandCreator {
}

@Component
public class CommandCreator2 implements CommandCreator {
}

@Component
public class CommandCreator3 implements CommandCreator {
}


@Component
public class CommandCreatorRegistry {

    private Map<String, CommandCreator> map;

    public CommandCreatorRegistry(@AutoWired List<CommandCreator> commandCreators) {

       // register commands in map

    }

}

Spring is only one example to achieve this. But if Spring might be out of scope of your project or too seen as too heavy then you may go with your own solution.