Java CLI Program Commands

Question

As I was creating a CLI program, I realized having a ton of switch/if-then statements was a messy way to process user input. So, I set up a way to dynamically create commands as I added modules (Classes that extend the Module class) and methods. When a user enters a command like "sys::get bios" it gets passed to the loaded Module with the ID "sys". The base functionality defined in Module goes through each method in the runtime class and selects the best matching method. In this case, it would find get_bios() and call it via reflection. I was wondering if anyone else has a way a improve this code, or expand its functionality.

Process Input Method:

public static void processInput(String input, boolean recordLast)
{
 if (input.startsWith("|"))
 {
 if (input.length() > 1) {
 List<String> output = Runner.runSimple("cmd.exe /c " + input.substring(1, input.length()));
 output.forEach(l -> Logger.logVerbose(l));
 }
 }
 else if (input.contains("::"))
 {
 //Command Syntax: MODULE::COMMAND ARGS
 if (ModuleRegistry.getModule(input.split(Pattern.quote("::"))[0]) != null)
 {
 ModuleRegistry.getModule(input.split(Pattern.quote("::"))[0]).run(input.substring(input.indexOf("::")+2));
 if (recordLast) lastInput = input;
 }
 else
 {
 Logger.logError("Invalid context: " + input.split(Pattern.quote("::"))[0]);
 }
 }
 else if (input.equals("!"))
 {
 if (!lastInput.equals("!")) processInput(lastInput);
 }
}

Module run method (Not overridden by a Module Class):

public void run(String input)
{
 List<String> args = new ArrayList<String>();
 Matcher m = argPattern.matcher(input);
 while (m.find()) args.add(m.group(1).replace("\"", "")); //Get all args and remove any surrounding quotes
 Method method = matchingMethod(args);
 if (method == null) //No matching valid command methods
 {
 Logger.logVerbose("Command not found");
 }
 else
 {
 if (SystemType.isMatch(BlackStar.systemType, method.getAnnotation(ModuleCommand.class).supportedSystems()))
 {
 boolean isPrivate = !method.isAccessible();
 try {
 method.setAccessible(true); //Just incase the method is private, we need to make it accessible to call it
 if (method.getParameterCount() > 0)
 {
 if (method.getParameterCount() == 1 && args.size() == 0) args.add(""); //If method expects 1 argument and we have none, add a blank string
 method.invoke(this, (Object[]) args.toArray(new String[0]));
 }
 else
 {
 method.invoke(this, new Object[0]);
 }
 method.setAccessible(isPrivate); //Reset accessibility
 } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {
 Logger.logError("Invalid Arguments");
 e.printStackTrace();
 }
 }
 }
}

Method Matcher:

public Method matchingMethod(List<String> arguments)
{
 //TODO Ignore methods that match the best match perfectly in case the super-class implementation is used instead of the overriden method.
 Method bestMatch = null;
 Class<?> superClass = this.getClass();
 while (!(superClass == null) && !(superClass.equals(Object.class))) //Traverse class inheritance until top of tree is reached. Ignore Object class
 {
 for (Method method : superClass.getDeclaredMethods())
 {
 if (method.isAnnotationPresent(ModuleCommand.class)) //Is this a method that is allowed to be run as a command?
 {
 String[] mName = method.getName().split(Pattern.quote("_")); //Use the underscore as a delimiter in method name. Example "list_shadows" would match up to "list shadows"
 if (mName[0].length() == 0) mName = ArrayUtil.subset(mName, 1, mName.length); //If method starts with "_" the first value is blank, and should be removed
 boolean match = true;
 for (int i = 0; i < Math.min(mName.length, arguments.size()); i++)
 {
 if (!mName[i].equals(arguments.get(i)))
 {
 match = false;
 }
 }
 if (match)
 {
 if (bestMatch == null || bestMatch.getName().split(Pattern.quote("_")).length < method.getName().split(Pattern.quote("_")).length)
 {
 //The found method is either the first match, or more of the arguments match the method name
 if (method.getName().split(Pattern.quote("_")).length + (method.getParameterCount()) == arguments.size()) {
 bestMatch = method;
 }
 }
 }
 }
 }
 superClass = superClass.getSuperclass();
 }
 if (bestMatch != null) //If we have found any match
 {
 String[] mTokens = bestMatch.getName().split(Pattern.quote("_"));
 if (mTokens[0].length() == 0) mTokens = ArrayUtil.subset(mTokens, 1, mTokens.length);
 //Now remove any arguments that matched method name from the list
 for (int i = 0; i < mTokens.length; i++) arguments.remove(0);
 }
 return bestMatch;
}

An example ModuleCommand in the Sys module:

@ModuleCommand(supportedSystems = SystemType.WINDOWS, friendlyName = "Get BIOS Info")
protected String get_bios()
{
 String ret = "";
 //Run command to get BIOS information
 return ret;
}

• Simon Forsberg: Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers .

Answer 1

Conventions & Style:

• Opening curly braces in Java don't go on a separate line
• The standard indent level is 4 spaces (or one tab)
• You're adhering to naming convention, good job.

---

• You should always place curly braces around blocks, because otherwise readability suffers. Places like if (recordLast) lastInput = input; and while (m.find()) args.add(m.group(1).replace("\"", "")); come to mind.
• In some places you have rather long lines. Java allows you to use linebreaks to make things more readable.
• I personally dislike end-of-line comments because they are easily pushed out. Also they often show places where you could (/should?) extract methods instead of commenting.

Platform Dependence

Runner.runSimple("cmd.exe /c " + input.substring(1, input.length()));

You're making yourself dependent on windows here. This is IMO not a good idea, but I assume it's exactly the point of the Program, so there's that. Beware that the way you implemented this makes it hard to allow running on another platform, though.

If you'd instead use a PlatformRunner interface you could switch the runners depending on which platform you're on. The System class in the standard Java Library already has the necessary tools for this with System.getProperty("os.name"). A runner could encapsulate the platform specific behaviour into separate classes and allow you to run this anywhere :)

While we're right there:

Intermediate Variable Overuse

 List<String> output = Runner.runSimple("cmd.exe /c " + input.substring(1, input.length()));
 output.forEach(l -> Logger.logVerbose(l));

could be just:

Runner.runSimple("cmd.exe /c " + input.substring(1, input.length()).forEach(Logger::logVerbose);

Let me circle back to what I mentioned above:

Extract Methods

The first example is one I already mentioned above: //Get all args and remove any surrounding quotes could just as well be:

List<String> args = extractArgs(input);

with:

private static List<String> extractArgs(final String input) {
 Matcher m = argPattern.matcher(input);
 List<String> result = new ArrayList<String>();
 while (m.find()) {
 // remove surrounding quotes
 result.add(m.group(1).replace("\"", "");
 }
 return result;
}

I'm sure you can find a few other places where you code has these logical units that can be extracted into separate methods.

Arrow Code Symptomatics

The next thing I noticed was the deep nesting and indentation you had.
Consider your run-method:

public void run(String input) {
 List<String> args = extractArgs(input);
 Method method = matchingMethod(args);
 if (method == null) {
 Logger.logVerbose("Command not found");
 } else {
 // lots and lots of code
 }
}

You can remove a level of indentation from your else-block by using an early return:

public void run(String input) {
 List<String> args = extractArgs(input);
 Method method = matchingMethod(args);
 if (method == null) {
 Logger.logVerbose("Command not found");
 return;
 }
 // lots and lots of code
}

This makes it easier to follow methods (because you don't have to keep track of mutliple scopes) and it allows you to keep lines longer before splitting them.

The same applies for nesting loops (as in your matchingMethod). You can use break to improve readability. Additionally extracting methods can allow you to reduce indentation.

Additional Notes

• You explicitly mention you're targeting Java 8. I don't see all that many Java 8 features in this code.
• You're commenting quite a lot. In most places the comments are a symptom that you need a method instead.

For the remainder your code is difficult to read because it follows C# conventions. I think it's reasonably clean, the only problem it exhibits is intermingling levels of abstraction. Don't be afraid to extract more methods. At the topmost level, code should read almost like prose. That makes it easier to grasp the gist of what the code does. You won't need the nitty gritty details every time you read the code. Account for that :)

• CConard96: Thanks. I wan't even aware of how much my VB background showed in my code until now. Breaking up the run() and matchingMethod() methods definitely made it easier to read. Also, this was supposed to be multi-platform but a I took a few steps back to support just Windows until I got a base functionality working right. The Runner class just spawns processes, so the only time I need to specify "cmd.exe /c" is when I try using built in commands. In the future I'll add a way to abstract this for other SystemTypes.

Answer 2

I've really only taken a deep look at the processInput method. Still, some comments:

1. If this function is invoked with just "::" as an input argument it will throw an ArrayIndexOutOfBoundsException exception.

2. The last if statement looks redundant. lastInput seems as if it can only be set in the else if (input.contains("::")) block, so can never be "!".

3. Split this up; it's too long and makes your code harder to follow and more error-prone:

   ModuleRegistry.getModule(input.split(Pattern.quote("::"))[0]).run(input.substring(input.indexOf("::")+2));
   

4. Remove the duplicate statement to make this method neater:

   ModuleRegistry.getModule(input.split(Pattern.quote("::"))[0]
   

5. Lots of static methods. Consider a more object-oriented approach.

I haven't looked too in depth at the reflection logic, but two comments after a high level look:

1. Wouldn't this be better as a Bash/Python script? Dispatching methods dynamically through Java reflection for general operating system specific tasks strikes me as the wrong tool for the job. Seems error prone, a security risk and an unnecessary level of indirection.

2. In order to expose a class to your framework you require it to be a subclass of module, effectively tying it to this type hierarchy. If you do keep this design, consider making run final as there's nothing to stop clients of your framework overriding this method.

• ferada: Welcome to Code Review and good job on your first answer, enjoy your stay!