Agree with rip-off 100%. As it is the code is really difficult to read and understand. This particular issue with the NullPointerException was relatively easy to find - the trace had the exact line number and I could just look at the row and check which references can potentially cause the exception. But had it been e.g. a logic issue with some processing results..
As a next step for refactoring I'd even suggest that you move the command processing completely outside the "command switch" if you're going to have many different commands. I'd even go there with the current number of commands as that allows for easier testing and re-use of the said commands. This could be something like this (extending on rip-off's excellent example):
public class CommandSwitch {
/**
* Map containing all supported commands.
*/
private final java.util.Map<String, CommandProcessor> commandMap;
public CommandSwitch() {
commandMap = new java.util.Hashtable<String, CommandProcessor>();
}
/**
* Adds a new command to the switch. This can be called from whatever container/controller
* you're having.
*/
public void addCommandProcessor(String command, CommandProcessor processor) {
commandMap.put(command, processor);
}
public void logError(String error) {
System.out.println("ERROR - " + error);
}
public void printUsage(CommandSender sender, String usage) {
System.out.println("To " + sender + ": " + usage);
}
public boolean onCommand(CommandSender sender, Command cmd, String label, String[] args) {
if (!(sender instanceof Player)) {
logError("Invalid sender: " + sender);
return false;
}
Player player = (Player) sender;
String commandName = cmd.getName();
if (!commandName.equalsIgnoreCase("mc_web")) {
printUsage(player, "Unrecognised command: " + commandName);
return false;
}
if (args.length < 2) {
printUsage(player, "Not enough arguments: " + args.length);
return false;
}
String target = args[0];
String verb = args[1];
CommandProcessor processor = commandMap.get(target);
if(processor != null) {
return processor.processCommand(target, verb);
}
printUsage(player, "Don't understand: " + target + " " + verb);
return false;
}
public static void main(String[] args) {
CommandSwitch handler = new CommandSwitch();
handler.addCommandProcessor("money", new MoneyCommand());
handler.addCommandProcessor("item", new ItemCommand());
handler.addCommandProcessor("account", new AccountCommand());
CommandSender sender = new Player();
handler.onCommand(sender, new Command("mc_web"), "Money Label", new String[] { "money", "deposit" } );
handler.onCommand(sender, new Command("mc_web"), "Item Label", new String[] { "item", "deposit" } );
handler.onCommand(sender, new Command("mc_web"), "Unknown", new String[] { "invalid", "deposit" } );
}
/**
* These should be outside the class, now internal just to make the example standalone
*/
public static interface CommandProcessor {
public boolean processCommand(String name, String verb);
}
public static class MoneyCommand implements CommandProcessor {
public boolean processCommand(String name, String verb) {
System.out.println("MONEY: " + name + ", " + verb);
return true;
}
}
public static class ItemCommand implements CommandProcessor {
public boolean processCommand(String name, String verb) {
System.out.println("ITEM: " + name + ", " + verb);
return true;
}
}
public static class AccountCommand implements CommandProcessor {
public boolean processCommand(String name, String verb) {
System.out.println("ACCOUNT: " + name + ", " + verb);
return true;
}
}
public static class CommandSender {
/* whatever */
}
public static class Player extends CommandSender {
/* whatever */
}
public static class Command {
private final String name;
public Command(final String name) {
this.name = name;
}
public String getName() {
return name;
}
/* whatever */
}
}
The code is still lacking, it doesn't e.g. verify input values for nullness, but should give you a good idea of what I'm proposing. The design is actually according to "Command" design pattern, but I wouldn't recommend getting wild with different design patterns yet.
1
