5

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?

  • 3
    I'm not sure I really understand what is so so bad about #1. Sure, maybe putting that code in your Main method is a bad practice, but what you've shown is basically the implementation of a of the factory pattern. This is a standard design pattern in enterprise systems. If you moved the code from #1 into a "CommandFactory" I think that it's probably a legitimate design. – mclark1129 Jul 6 '17 at 11:56
  • Interesting observation, that I received answers for a similar question (different language though) softwareengineering.stackexchange.com/questions/351389/… , which are almost opposite to those here. – Roman Susi Jul 6 '17 at 12:45
  • 1
    Reflection seems unsuitable and ridiculous. The so called "principle" is more of a misunderstanding than anything; either of the other solutions is fine. – Frank Hileman Jul 6 '17 at 16:46
  • @mclark1129 It's not that it's bad, it's just that #2 is slightly better in several different ways. The biggest reason is that in the future, it would be possible to add commands in a different place with something along the lines of commandDispatcher.addCommand("command37", Command37::new). – The Guy with The Hat Jul 6 '17 at 16:54
  • 1
    @mclark1129 #1 is bad because Java allows fallthough. It's easy to create a bug where you get the wrong command back because you forgot a break. #2 solves that problem. Not a big deal if you're returning from the conditional however. There are situations where the Dictionary is better, but they're equivalent here. – RubberDuck Jul 7 '17 at 10:53
18

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).

  • 1
    Agreed. In cases like these, you should take a long, hard, look at what the OCP is giving you, and what it is costing. If you're not going to allow plugins to add new commands, #2 will serve you well. It has the added benefit of taking almost no time to implement, in case you do eventually need something more fancy. – Jacob Raihle Jul 6 '17 at 11:06
12
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.

  • Factory is the correct answer here. – Eternal21 Jul 11 '17 at 18:30
  • Though a good choice for some abstraction, this doesn't answer the question: what would the underlying code do? – user949300 Sep 25 '18 at 19:58
  • 1
    Sure it does @user949300. It doesn’t matter. Just pick one. Flip a coin if you have to. Software is soft. You can always change it later. – RubberDuck Sep 25 '18 at 20:04
  • 1
    If somebody asked "should I use mergesort or shellsort or foosort", (with some technical explanation) and the answer was "just use a sort factory and then flip a coin", would you consider that a good answer? – user949300 Sep 25 '18 at 20:43
  • Honestly, yes. The implementation is a detail I don’t care about until I have a reason to believe I need a better one. Use bubblesort (i.e. the simplest to implement) @user949300. If you have performance, or other, problems later, then you can benchmark and improve the implementation. However, it’s only easy enough to do that if you already have a good abstraction in place. – RubberDuck Sep 25 '18 at 20:47
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", [] );
  • I certainly don't recommend gold-plating all your code to make it reusable, but I think that would be preferable to going out of your way to ensure your code is not reusable as this approach seems to do. The only positive I can see from taking this route is that it is quick to implement. – Dunk Jul 11 '17 at 18:48
2

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)

  • Great answer, as it allows a sophisticated end user to extend the app by editing a text file and adding classes to the classpath. Exactly what OCP is talking about. – user949300 Sep 25 '18 at 20:01
2

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

1

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.

  • 5
    Confucius say, "Don't shoot fly with cannon." – RubberDuck Jul 7 '17 at 10:58
  • You should let the interrogator decide what he/she needs to fight and what he/she needs for that. – oopexpert Jul 7 '17 at 11:08
  • @oopexpert for an undergraduate level software engineering project, Spring is (1) massive overkill and (2) probably outside of the rules of the project, which will have been designed to show what the students are able to produce themselves, not how adept at reusing other people's code they are. But implementing this yourself via reflection is a pain that would provide little to no benefit to the future maintainability of their code, as such code has a habit of being difficult to change yet not flexible enough for future requirements, which is often a pain point later. DTSTTCPW, as they say. – Jules Jul 10 '17 at 18:37
  • (1) What would be a measurement to be a "massive overkill"? (2) Maybe it is outside of the rules of the project to use Spring. But the task remains: Register all commands available without touching any existing code. Spring is only one way to address this task of inversion of control. I chose Spring DI as a well-known representation of IoC. But you can also go with something else that behaves similar. The mechanism is important not the technology. – oopexpert Jul 11 '17 at 0:46

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.