Writing a cleaner and more modular command-parser

Question

I'm writing a debugger for a Z80-emulator we are writing in a school project, using Java. The debugger reads a command from the user, executes it, reads another command, etc.

Commands can either be argument less, have optional arguments, or take an unlimited amount of arguments. Arguments are mostly integers, but occasionally they're strings.

Currently, we're using the Scanner-class for reading and parsing input. The read-method looks kinda like like this (I'm writing this off the top of my head, not paying attention to syntax nor correctness).

This was a kludge written in the beginning of the project, which quickly got out of hand as we added more and more commands to the debugger.

The major issues I have with this code is the large amount of repetition, the highlevel of if/else-nestedness, and the all around uglyness.

I would like suggestions on how to make this code more beautiful and modular, and what kind of patterns that are suitable for this kind of program.

I would also like more general suggestions on code style.

Answer 1

Have you looked at doing the dispatching with a Map? A hashmap would be pretty easy to put in there. Just make the key the command and make an interface or abstract class that is a command like this:

interface Commmand {
   void execute(String args);
}

Or even better you could chop up the arguments in advance:

interface Commmand {
   void execute(String[] args);
}

Then your you would use HashMap<String,Command>.

Answer 2

yup, there is a simpler/better way, especially in Java or other OO languages.

The basic insight, first, is that your command parser is a finite state machine: the START state is an empty line (or index at the start of a line).

Let's think about echo:

$ echo foo bat "bletch quux"

1. tokenize the line into pieces:

   "echo" "foo" "bar" "bletch quux"

2. in a shell, the grammar is usually verb noun noun noun... so interpret it that way. You CAN do it with a sequence of if-else if things, but a hash is better. You load the hash with strings as indices, and index something else. It could be just a number, which would go into a switch:

(this is pseudocode):

  Hashtable cmds = new Hashtable();
  enum cmdindx { ECHO=1, LS=2, ...}
  cmds.add("echo", ECHO); // ...

  // get the token into tok
  switch (cmds.get(tok)) {
     case ECHO: // process it
       // get each successor token and copy it to stdout
       break;
     ...
     default:
        // didn't recognize the token
        // process errors
   }

EVEN better, you can apply the Command and Object Factory patterns. Now you have a class Command

  public interface Command {
     public void doThis(String[] nouns) ;
     public Command factory();
  }

  public class Echo implements Command {
     public void doThis(String[] nouns){
         // the code is foreach noun in nouns, echo it
     }
     public Command factory(){
         // this clones the object and returns it
     }
  }

Now, your code becomes

  // Load the hash
  Hashtable cmds = new Hashtable();
  cmds.add("echo", new Echo());  // one for each command


  // token is in tok
  // the "nouns" or "arguments are in a String[] nouns
  ((cmds.get(tok)).factory()).doThis(nouns);

See how this works? You look up the object in the hash. You call the factory method to get a new copy. You then invoke the processing for that command using the doThis method.

Update

This may be a bit too general, in that it uses the Factory pattern. Why have a factory method? Mainly, you'd use that so that each time you execute a command, the "verb" object (like the instance of Echo) can have its own internal state. If you don't need state to persist for a long time, you can simplify this to

  (cmds.get(tok)).doThis(nouns);

Now it simply gets and uses the Echo object you created when you instanciated it with cmds.add("echo", new Echo());.