Given that you appear to be loading this plugin into a server, you should carefully consider the security of the host when handling any user-submitted data. For example, what if a malicious user creates a user name containing directory climbing references, such as "../anotheruser". Can the user submit newline delimited text to the server? If so, the user might be able to subvert your file format by inserting newlines in unexpected locations.
Do not rely on Minecraft to validate anything for you - a malicious user might have a modified client or insert a proxy between the client and server which might allow them to break lots of rules you might expect to be enforced.
Failure to correctly handle security can not only cause the loss of the plugin data, but in extreme cases could allow other data on the host to be compromised or deleted, worst case scenario the host might be remotely compromised.
Another issue is data integrity, if this is a serious project you should strongly consider using an existing robust database system rather than manually implementing an ad-hoc database on top of flat files.
Finally, your code is extremely confusing because you've packed all the logic in one enormous method. Consider introducing private helper methods. The dispatch method could look like this:
public boolean onCommand(CommandSender sender, Command cmd, String label, String[] args) {
if (!(sender instanceof Player)) {
logError("Invalid sender: " + sender);
return whatever;
}
Player player = (Player) sender;
String commandName = cmd.getName();
if (!commandName.equalsIgnoreCase("mc_web")) {
printUsage(player, "Unrecognised command: " + commandName);
return whatever;
}
There is a lot more that could be said about the current coding style in use... I actually find it more or less impossible to read coherently, I can only guess at the number of bugs it is likely to contain. It should have been split long ago.
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.
