The best way to write this Java code?
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 piece of server code to handle post types. Each message contains a type in commandArr[0]
and parameters in the rest commandArr[]
. However, this current code seems very inelegant at runtime. Is there a better way to handle this? (As far as I know, values String
cannot be used in operators switch
, and even then the operator switch
will only be a small improvement.
source to share
I would refactor this using the Command Design Pattern .
Basically each of your commands, messages, quit, confirmconnect and by default will have a class implementation and will implement a command line 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 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);
}
source to share
Here are two options using enums that (almost) provide the same behavior in a more readable way:
1) Enumerations for type safe:
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 advantages are that the code is more readable, but you avoid creating new methods or classes for each of the cases, which prevents you from doing what you want if the processing code only has one or two lines.
2) Another enum-based option, which is actually a Command pattern, but which contains a lot of 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);
}
source to share
Yeap looks like Command + Prototype to me.
In a command, you define what will be done, and the prototype must put an instance of each command in the lookup table and "clone" them to execute each time.
The refactoring will look like this:
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 provide concrete 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...
source to share
Take a look at the Commons CLI which is a command line argument parser.
Here are some examples of its use.
source to share
First, I would make a map between commands and a class that executes each type of command (say, an anonymous class that implements a known interface) and then retrieves the correct class from the map and then passes it to the rest of the parameters.
If that makes sense, you can use the enumeration here with a static method to get the correct one, that way you could switch if and when you need to (let's say you needed to do the same for 5 out of 10 commands).
source to share
I like Bob. Another method would be to use Spring framework and IoC functionality. Basically, I've done this before to use Spring (inflated from xml) to create a map where you have each command object stored with a key. The key will be the same as the text in commandArr[0]
.
So your xml looks something like this:
<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 avoids running any initialization code. All you have to do is inflate the xml. Spring handles the map for you. Alternatively, you can define any required data members in the classes using the same syntax. Also, if you need to add functionality, all you have to do is change the xml, not dump and recompile the code. I am personally a fan of huuuuge :)
For more information, check out this article for a quick overview of IoC, then check out this article for documentation
source to share