Better way to write this Java code?

Question

public void handleParsedCommand(String[] commandArr) {
    if(commandArr[0].equalsIgnoreCase("message")) {
        int target = Integer.parseInt(commandArr[1]);
        String message = commandArr[2];
        MachatServer.sendMessage(target, this.conId, message);
    } else if(commandArr[0].equalsIgnoreCase("quit")) {
        // Tell the server to disconnect us.
        MachatServer.disconnect(conId);
    } else if(commandArr[0].equalsIgnoreCase("confirmconnect")) {
       // Blah blah and so on for another 10 types of command
    } else {
        try {
            out.write("Unknown: " + commandArr[0] + "\n");
        } catch (IOException e) {
            System.out.println("Failed output warning of unknown command.");
        }
    }
}

I have this part of my server code for handling the types of messages. Each message contains the type in commandArr[0] and the parameters in the rest of commandArr[]. However, this current code, while working seems very unelegant. Is there a better way to handle it? (To the best of my knowledge, String values can't be used in switch statements, and even then, a switch statement would only be a small improvement.

Answer 1

I'd refactor this using the Command Design Pattern.

Basically each of your commands, message, quit, confirmconnect and a default will have a class implementation and will implement the Command Interface.

/*the Command interface*/
public interface ICommand {
    void execute(String[] commandArr);
}

public class Message implements ICommand {
    void execute(String[] commandArr) {
        int target = Integer.parseInt(commandArr[1]);
        String message = commandArr[2];
        MachatServer.sendMessage(target, this.conId, message);
    }
}

//same type of class for other commands

public class CommandManager {
    public ICommand getCommand(String commandName) {
        //store all the commands in a hashtable. 
        //Pull them out by name as requested.
    }

    //Esko's suggestion from comments
    public static void executeImmediately(String[] commandArr) {
        getCommand(commandArr[0]).execute(commandArr);
    }
}


public void handleParsedCommand(String[] commandArr) {

    ICommand command = CommandManager.getCommand(commandArr[0]);
    command.execute(commandArr);

//or Esko

    CommandManager.executeImmediately(commandArr);

}

Answer 2

Take a look at Commons CLI which is a command-line argument parser.

Here are some examples of its usage.

Answer 3

You can use enums

Answer 4

For starters, I would make a map between the commands and a class which executes each type of command (say an anonymous class that implements a known interface) and then retrieve the right class from the map, and then passes it the rest of the parameters.

If it made sense, you could use an enum here with a static method to retrieve the right one, that way you could switch if and when you needed to (say you had to do the same thing on 5 of the 10 commands).

Answer 5

First of all you are reading the same element of the array every time. This should be the first thing to factor out. equalsIgnoreCase is a bit long, so normalise the case first (don't pick up the default locale!).

It is possible to use enums to hack a switch of Swtings. JDK7 may include a switch on String, IIRC.

Answer 6

I like Bob's answer. Another method would be to use the Spring framework and the IoC functionality. Basically, I've done this before to use Spring (inflates from xml) to create a Map where you have each command object stored with a key. The key would be the same as the text in commandArr[0].

So your xml looks something like

<property name="commands">
  <map>
    <entry>
        <key>
           <value>message</value>
        </key>
        <ref bean="messageCommand" />
     </entry>
  </map>
 </property>
 <bean id="messageCommand" ref="org.yourorg.yourproject.MessageCommand" />

And then in your code...

commands.get(commandArr[0]).execute() //or whatever

This allows you to not run any sort of initialization code. All you have to do is inflate the xml. Spring handles populating the map for you. Also, you can define any necessary data members in the classes using a similar syntax. Also, if you need to add functionality, all you have to do is change the xml rather than mucking with and recompiling code. I'm personally a huuuuge fan :)

For more info, check out this article for a brief overview of IoC and then check out this article for the documentation

Answer 7

Here are two variants using enums that (nearly) provide the same behavior in a much more readable way:

1) Enums for a type-safe switch:

enum CommandType {
MESSAGE,
QUIT,
CONFIRMCONNECT
}

public void handleParsedCommand(String[] commandArr) {
    CommandType cmd = null;

    try {
        cmd = CommandType.valueOf(commandArr[0].toUpperCase());
    } catch (IllegalArgumentException e) {
        // this kind of error handling, seems a bit strange, by the way.
        try {
            out.write("Unknown: " + commandArr[0] + "\n");
        } catch (IOException e) {
           System.out.println("Failed output warning of unknown command.");
        }
        return;
    }
    switch(cmd) {
        case MESSAGE:
            int target = Integer.parseInt(commandArr[1]);
            String message = commandArr[2];
            MachatServer.sendMessage(target, this.conId, message);
        case QUIT:
            // Tell the server to disconnect us.
            MachatServer.disconnect(conId);
        case CONFIRMCONNECT:
            // Blah blah and so on for another 10 types of command
        }
    }
}

The main benefits are that the code is more readable, but you avoid creating new methods or classes for each of the cases, which is not allows what you want if the handling code has only one or two lines.

2) Another enum-based variant, that is in fact a Command pattern, but which out much bloat code:

enum CommandType {
 MESSAGE {
  void execute(CommandProcessor cp, String[] params) {
      int target = Integer.parseInt(params[1]);
      String message = params[2];
      MachatServer.sendMessage(target, cp.conId, message);  
  }
 },
 QUIT {
  void execute(CommandProcessor cp, params param) {
      MachatServer.disconnect(cp.conId); 
  }
 },
 CONFIRMCONNECT {
  void execute(CommandProcessor cp, params param) {
     // Blah blah and so on for another 10 types of command
  }
 };

 abstract void execute(CommandProcessor cp, String[] param);
}
public void handleParsedCommand(String[] commandArr) {
 CommandType cmd = null;

 try {
  cmd = CommandType.valueOf(commandArr[0].toUpperCase());
 } catch (IllegalArgumentException e) {
     try {
         out.write("Unknown: " + commandArr[0] + "\n");
     } catch (IOException e) {
         System.out.println("Failed output warning of unknown command.");
     }
     return;
 }
 cmd.execute(this, commandArr);
}

Answer 8

Yeap, looks like a Command + Prototype pattern to me.

In the command you define what is going to be done, and the prototype is to place an instance of each command in a lookup table and "clone" them to be executed each time.

The refactoring would be like:

Before:

public void handleParsedCommand(String[] commandArr) {
        if(commandArr[0].equalsIgnoreCase("message")) {
        int target = Integer.parseInt(commandArr[1]);
        String message = commandArr[2];
        MachatServer.sendMessage(target, this.conId, message);
    } else if(commandArr[0].equalsIgnoreCase("quit")) {
        // Tell the server to disconnect us.
        MachatServer.disconnect(conId);
    } else if(commandArr[0].equalsIgnoreCase("confirmconnect")) {
       // Blah blah and so on for another 10 types of command
    } else {
        try {
            out.write("Unknown: " + commandArr[0] + "\n");
        } catch (IOException e) {
            System.out.println("Failed output warning of unknown command.");
        }
    }
}

After:

public void handleParsedCommand(String[] commandArr) {
    Command.getCommand( commandArr ).execute();
}


// Define the command and a lookup table
abstract  class Command {

    // Factory using prototype 
    public static Command getCommand( String [] commandArr ) {
        // find the handling command 
        Command commandPrototype = commandMap.get( commandArr[0] );
        // if none was found, then use "uknown"
        if ( commandPrototype == null ) {
             commandPrototype = commandMap.get("unknown");
        }
        // Create an instance using clone
        Command instance = commandPrototype.clone();
        instance.args = commanrArr;
        return instance;

    }

    // lookup table ( switch substitute )
    private static Map<String,Command> commandsMap = new HashMap()<String,Command>(){{
          put("message"       , new MessagCommand());
          put("quit"          , new QuitCommand());
          put("confirmconnect", new ConfirmConnectCommand());
          ...
          put("unknow"        , new UnknownCommand());

    }};


    // args of the command
    private String [] args;


    public void execute();

    String [] getArgs(){
        return this.args;
    }


}

And the provide the specific implementations

class MessageCommand extends Command {
     public void execute(){
        int target = Integer.parseInt(commandArr[1]);
        String message = commandArr[2];
        MachatServer.sendMessage(target, this.conId, message);
     }
}

class MessageCommand extends Command {
     public void execute(){
        int target = Integer.parseInt(getArgs()[1]);
        String message = getArgs()[2];
        MachatServer.sendMessage(target, this.conId, message);
     }
}

class QuitCommand extends Command {
     public void execute() {
      MachatServer.disconnect(conId);
     }
 }

 class ConfirmConnectCommand extends Command {
     public void execute() {
     /// blah blah blah 
     }
 }
 class UnknowCommand extends Command  {
     public void execute() {
         try {
            out.write("Unknown: " + commandArr[0] + "\n");
         } catch (IOException e) {
            System.out.println("Failed output warning of unknown command.");
         }
     }
 }

 // ... other 10 implementations here...