FollowUp CommandLineInterpreter (CommandLineInteraction)

Question

I wrote an command line interpreter (CLI) in Java and got some real helpful hints on how to improve.

The cli is responsible to pick up the users input (via keyboard aka command line) into any app that implements the CommandLineInterpreter.

The app itself offers commands to the CLI that can be executed. If the user types in a proper command the CLI executes that command on the app.

Note: i changes the name from CommandLineInterface into CommandLineInteraction to prevent an misinterpreting naiming (i wished i could speak english)

Can you review this code on

• Design
• Clean code
• OO
• Naming (my bad englisch leads to bad naming)
• i'm not sure if i have too much aggregation and hard coupling here
• how do i test this application?

class Command

any command must subclass from this command so the CLI can execute the command when typed in. The command itself is now responsible to call the right methods on it's Application

public abstract class Command<Application> {
 private final String identifier;
 private final Application application;
 public Command(Application application, String identifier) {
 this.application = application;
 this.identifier = identifier;
 }
 public Application getApplication() {
 return application;
 }
 public abstract Response execute(List<String> parameter);
 public String getIdentifier() {
 return identifier;
 }
 public boolean isIdentifier(String ident) {
 return identifier.equals(ident);
 }
 //lr'dr 
 //this class overrides equals and hashCode on sole parameter "identifier"
}

class CommandLineInteraction

this class is responsible to take the user input, find the correspondig command and execute it finally... (yes sounds like too much responsibility for one class but it could be rephrased as handleInput)

public class CommandLineInteraction implements CommandLineInterpreter {
 private static final String COMMAND_SEPARATOR = " ";
 private static final String COMMAND_PROMPT = "$>";
 private final CommandLineInterpreter cli;
 private final CommandLineInteractionInterpreter commandLineInteractionInterpreter;
 private final InputStream input;
 private final PrintStream output;
 private boolean isRunning = true;
 public CommandLineInteraction(CommandLineInterpreter cli, InputStream input, PrintStream output) {
 commandLineInteractionInterpreter = new CommandLineInteractionInterpreter(this);
 if (cli == null || commandLineInteractionInterpreter.hasCommandInCommon(cli)) {
 throw new RuntimeException("CommandLineInterpreter interface of " + cli + " is not properly implemented");
 }
 this.cli = cli;
 this.input = input;
 this.output = output;
 }
 @Override
 public Set<Command> getCommands() {
 return commandLineInteractionInterpreter.getCommands();
 }
 public void start() {
 Scanner scanner = new Scanner(input);
 showHelp();
 while (isRunning) {
 output.print(COMMAND_PROMPT);
 String line = scanner.nextLine();
 List<String> words = Arrays.asList(line.split(COMMAND_SEPARATOR));
 String identifier = words.get(0);
 List<String> parameters = words.subList(1, words.size());
 Optional<Command> command = findCommand(identifier);
 if (command.isPresent()) {
 Response response = command.get().execute(parameters);
 if (response.failed()) {
 output.println(response);
 }
 } else {
 showHelp();
 }
 }
 }
 private Optional<Command> findCommand(String identifier) {
 return getAllCommands().stream().filter(c -> c.isIdentifier(identifier)).findAny();
 }
 private Set<Command> getAllCommands() {
 Set<Command> commands = cli.getCommands();
 commands.addAll(commandLineInteractionInterpreter.getCommands());
 return commands;
 }
 public void showHelp() {
 Set<Command> commands = getAllCommands();
 output.println("help - these commands are available:");
 commands.forEach(c -> output.printf(" - %s\n", c.getIdentifier()));
 }
 public void stop() {
 isRunning = false;
 }
}

well honestly this class looks now far more readable - but i must confess i still carry the Set with this class and not the Map - this issue is still open and not to be mentioned as review finding. I'm still not sure if this really helps in here. maybe sleep over a night or so...

class CommandLineInteractionInterpreter

provides the two baisc command help and exit...

public class CommandLineInteractionInterpreter implements CommandLineInterpreter {
 private final Set<Command> cliCommands;
 CommandLineInteractionInterpreter(CommandLineInteraction commandLineInterface) {
 super();
 cliCommands = new HashSet<>();
 cliCommands.add(new HelpCommand(commandLineInterface));
 cliCommands.add(new ExitCommand(commandLineInterface));
 }
 @Override
 public Set<Command> getCommands() {
 return cliCommands;
 }
 boolean hasCommandInCommon(CommandLineInterpreter cip) {
 return !Collections.disjoint(cip.getCommands(), getCommands());
 }
}

I also changed the responsibility on execution into the command - away from the interpreter. That leads to the new CommandlineInterpreter whose quite naked now ^^

interface CommandLineInterpreter

public interface CommandLineInterpreter {
 Set<Command> getCommands();
}

so noetworhty, the new responsibility on execution lies on the command:

classes ExitCommand & HelpCommand

public class ExitCommand extends Command<CommandLineInteraction> {
 public ExitCommand(CommandLineInteraction commandLineInteraction) {
 super(commandLineInteraction, "exit");
 }
 @Override
 public Response execute(List<String> parameter) {
 getApplication().stop();
 return Response.success();
 }
}
public class HelpCommand extends Command<CommandLineInteraction> {
 public HelpCommand(CommandLineInteraction commandLineInteraction) {
 super(commandLineInteraction, "help");
 }
 @Override
 public Response execute(List<String> parameter) {
 getApplication().showHelp();
 return Response.success();
 }
}

• Martin Frank: i think the Intrepreter should REALLY reaturn a first-class collection (like a CommandMap) - that would solve many problems, that still are in that code
• Martin Frank: i've added that - updated code is at github.com/martinFrank/commandLineInterface
• Martin Frank: yes yes - give Set<Command> a home, it's done but not visible here =)

Answer 1

Maybe One of Us has the Wrong Idea

When I work with the code I gets confused by the CommandLineInterpreter and CommandLineInteraction, because currently a CommandLineInteraction is a CommandLineInterpreter. I think actually a CommandLineInteraction has a CommandLineInterpreter

Then I looked into the CommandLineInterpreter:

public interface CommandLineInterpreter {
 Set<Command> getCommands();
}

but by the name I thought to found

public interface CommandLineInterpreter {
 Command interpret(String lineOfInput);
}

With this interface the start in CommandLineInteraction would look like

public void start() {
 Scanner scanner = new Scanner(input);
 showHelp();
 while (isRunning) {
 output.print(COMMAND_PROMPT);
 String line = scanner.nextLine();
 Command command = interpreter.interpret(line);
 // ..
 }
}

---

Type Embedded in Name

Avoid placing types in method names; it's not only redundant, but it forces you to change the name if the type changes.

In every place I can read command. Imagine you want to change the name to task you have to change at so many places..

Prefer schedule.add(course) to schedule.addCourse(course)

---

Type Parameter Naming Conventions

The most commonly used type parameter names are:

• E - Element (used extensively by the Java Collections Framework)
• K - Key
• N - Number
• T - Type
• V - Value
• S,U,V etc. - 2nd, 3rd, 4th types

The class signature of Command do not follow the convention

public abstract class Command<Application>

This is not bad, but first I thought Application would be a class name and searched for it. If you really want to use a type parameter more than one letters, than write them all in UPPERCASE.

In your previous question you wrote

Any class that implements this interface can use the [CommandLineInteraction]. [Application] is the application, which implements the CommandLineInteraction.

This is not the case still your type parameter Application is not a bounded type parameter. This means that I can create the following Commands:

Command<Integer>
Command<String>
Command<CustomTypeXY>

If you do not want to have this behavior, you can add a upper bound with the key word extends for a class or implements for an interface.

public abstract class Command<T extends CommandLineInteraction>

This means that a T must be a subtype or a CommandLineInteraction.

But if you have not the flexibility and it will only be a CommandLineInteraction you do not need a generic parameter at this point.

---

Create an Instance Variable in CommandLineInteraction

In CommandLineInteraction we can find a method getAllCommands. This method is used in two other methods: findCommand and showHelp. getAllCommands creates on each call a new Set with the commands of the cli and the commands of commandLineInteractionInterpreter.

Can the commands change? I think not because I find now methods, which add new commands at runtime. If this is true, create a instance variable commands in CommandLineInteraction instead of treat the method getAllCommands as an instance variable.

public class CommandLineInteraction implements CommandLineInterpreter {
 // ..
 Set<Command> commands;
 public CommandLineInteraction(CommandLineInterpreter cli, InputStream input, PrintStream output) {
 // ..
 this.commands = getAllCommands();
 }

---

What is a CommandLineInteractionInterpreter

provides the two basic command help and exit

The name of CommandLineInteractionInterpreter is confusing and from the quote and the code you can see that it is not more than a Factory, that gives you default commands.

---

Give Set<Command> a Home

A First Class Collection:

Any class that contains a collection should contain no other member variables. Each collection gets wrapped in its own class, so now behaviors related to the collection have a home.

Create It

class CommandColletion {
 private Set<Command> values;
 public add(Command) {/* ... */}
 public union(CommandColletion values) {/* ... */}
 public contains(CommandColletion values) {/* ... */}
}

Use it

class CommandLineInteraction implements CommandLineInterpreter {
 // ..
 CommandCollection commands;
 CommandLineInteraction(CommandLineInterpreter cli, InputStream input, PrintStream output, CommandCollection defaults) {
 // ..
 this.commands = defaults.union(cli.getCommands());
 }

---

And now you can safely change Set to Map :P

• Martin Frank: using a FirstClass Collection is definitely a TODO here! it's already done
• Martin Frank: my english my english - it's taking me to grave =) it's one root of my problems
• Martin Frank: how would you name a class that provides the commands?
• Roman: It depends.. If you implement it based on the FactoryPattern, than something like DefaultCommandFactory. But if pass the defaults through the constructor you maybe do not need a Factory.
• Martin Frank: OK - the proper naming has been done - the CommandLineInterpreter is responsible to find the proper command and execute this command. The CommandProvider is responsible to provide the right set of commands (as class CommandList, not as Set<Commands>)