Return to Answer

• 1

• 25.5k
• 7
• 59
• 141

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