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;
}

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

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.