Designing a Bukkit plugin framework - Child command handling via annotations

Question

Some words to introduce the situation.

Context: To ease my workflow while writing Bukkit plugins (the basically de-facto API for the Minecraft Server until Sponge gets it's implementation going), I've decided to put together a "mini-framework" for myself to not have to repeat the same tasks over and over again. (Also, I'm trying to design it to not depend too much on Bukkit, so I can continue using it on Sponge by just changing my implementation)

Intention: Command handling in Bukkit is, frankly, a mess. You have to define your root command (for example, you want to run /test ingame, "test" is the root) in a YML file (instead of calling some sort of factory?), child command handling is nonexistant and implementation details is hidden so producing 100% reliable results is hard. It's the only part of Bukkit that has annoyed me, and it was the main initiator of me deciding to write a framework.

Goal: Abstract away the nasty Bukkit command handling, and replace it with something that's clean.

---

Working towards it:

This is going to be the long paragraph where I'm going to explain how Bukkit command handling is originally implemented, as that will give a deeper understanding of important command parameters and such.

Any user connected to a Minecraft server can start a chat message with '/', which will result in it being parsed as a command.

To offer an example situation, any player in Minecraft has a life bar, which defaults to capping at 10 hearts, and depletes when taking damage. The maximum and current "hearts" (read: health) may be set by the server at any time.

Lets say we want to define a command like this:

/sethealth <current/maximum> <player or * for all> <value>

To start implementing this...oh boy. If you like clean code, I'd say skip this...I'll comment to explain, and whenever I feel like Bukkit did a mistake.

The mandatory plugin.yml:

# Full name of the file extending JavaPlugin
# My best guess? Makes lazy-loading the plugin possible
# (aka: just load classes that are actually used by replacing classloader methods)
main: com.gmail.zkfreddit.sampleplugin.SampleJavaPlugin

# Name of the plugin.
# Why not have this as an annotation on the plugin class?
name: SamplePlugin

# Version of the plugin. Why is this even required? Default could be 1.0.
# And again, could be an annotation on the plugin class...
version: 1.0

# Command section. Instead of calling some sort of factory method...
commands:
    # Our '/sethealth' command, which we want to have registered.
    sethealth:
        # The command description to appear in Help Topics
        # (available via '/help' on almost any Bukkit implementation)
        description: Set the maximum or current health of the player

        # Usage of the command (will explain later)
        usage: /sethealth <current/maximum> <player/* for all> <newValue>

        # Bukkit has a simple string-based permission system, 
        # this will be the command permission
        # (and as no default is specified,
        # will default to "everybody has it")
        permission: sampleplugin.sethealth

The main plugin class:

package com.gmail.zkfreddit.sampleplugin;

import org.bukkit.command.PluginCommand;
import org.bukkit.plugin.java.JavaPlugin;

public class SampleJavaPlugin extends JavaPlugin {

    //Called when the server enables our plugin
    @Override
    public void onEnable() {
        //Get the command object for our "sethealth" command.
        //This basically ties code to configuration, and I'm pretty sure is considered bad practice...
        PluginCommand command = getCommand("sethealth");

        //Set the executor of that command to our executor.
        command.setExecutor(new SampleCommandExecutor());
    }
}

The command executor:

package com.gmail.zkfreddit.sampleplugin;

import org.bukkit.Bukkit;
import org.bukkit.command.Command;
import org.bukkit.command.CommandExecutor;
import org.bukkit.command.CommandSender;
import org.bukkit.entity.Player;

public class SampleCommandExecutor implements CommandExecutor {

    private static enum HealthOperationType {
        CURRENT,
        MAXIMUM;

        public void executeOn(Player player, double newHealth) {
            switch (this) {
                case CURRENT:
                    player.setHealth(newHealth);
                    break;
                case MAXIMUM:
                    player.setMaxHealth(newHealth);
                    break;
            }
        }
    }

    @Override
    public boolean onCommand(
            //The sender of the command - may be a player, but might also be the console
            CommandSender commandSender,

            //The command object representing this command
            //Why is this included? We know this is our SetHealth executor,
            //so why add this as another parameter?
            Command command,

            //This is the "label" of the command - when a command gets registered,
            //it's name may have already been taken, so it gets prefixed with the plugin name
            //(example: 'sethealth' unavailable, our command will be registered as 'SamplePlugin:sethealth')
            String label,

            //The command arguments - everything after the command name gets split by spaces.
            //If somebody would run "/sethealth a c b", this would be {"a", "c", "b"}.
            String[] args) {
        if (args.length != 3) {
            //Our command does not match the requested form {"<current/maximum>", "<player>", "<value>"},
            //returning false will, ladies and gentleman...

            //display the usage message defined in plugin.yml. Hooray for some documented code /s
            return false;
        }

        HealthOperationType operationType;
        double newHealth;

        try {
            //First argument: <current/maximum>
            operationType = HealthOperationType.valueOf(args[0].toUpperCase());
        } catch (IllegalArgumentException e) {
            return false;
        }

        try {
            //Third argument: The new health value
            newHealth = Double.parseDouble(args[2]);
        } catch (NumberFormatException e) {
            return false;
        }

        //Second argument: Player to operate on (or all)
        if (args[1].equalsIgnoreCase("*")) {
            //Run for all players
            for (Player player : Bukkit.getOnlinePlayers()) {
                operationType.executeOn(player, newHealth);
            }
        } else {
            //Run for a specific player
            Player player = Bukkit.getPlayerExact(args[1]);

            if (player == null) {
                //Player offline
                return false;
            }

            operationType.executeOn(player, newHealth);
        }

        //Handled successfully, return true to not display usage message
        return true;
    }
}

Now you may understand why I'm choosing to abstract the command handling away in my framework. I don't think I'm alone in thinking that this way is not self-documenting and handling child commands this way does not feel right.

---

My Intention:

Similiar to how the Bukkit Event System works, I want to develop a framework/API to abstract this away.

My idea is annotating command methods with a respective annotation that includes all neccassary information, and use some sort of registerer (in the event case: Bukkit.getPluginManager().registerEvents(Listener, Plugin)) to register the command.

Again similiar to the Event API, command methods would have a definied signature. As dealing with multiple parameters is annoying, I decided to pack it all in a context interface (also, this way I do not break all previous code in case I need to add something to the context!). However, I also needed a return type in case I want to display the usage quickly (but I'm not going to pick a boolean, that's for sure!), or do some other stuff. So, my idea signature boils down to CommandResult <anyMethodName>(CommandContext).

The command registration would then create the command instances for annotated methods and register them.

My basic outline took form. Note that I haven't came around to writing JavaDoc yet, I added some quick comments on not self-documenting code.

Command registration:

package com.gmail.zkfreddit.pluginframework.api.command;

public interface CommandRegistration {

    public static enum ResultType {
        REGISTERED,
        RENAMED_AND_REGISTERED,
        FAILURE
    }

    public static interface Result {
        ResultType getType();

        //For RENAMED_AND_REGISTERED
        Command getConflictCommand();

        //For FAILURE
        Throwable getException();

        //If the command got registered in some way
        boolean registered();
    }

    Result register(Object commandObject);

}

The command result enumeration:

package com.gmail.zkfreddit.pluginframework.api.command;

public enum CommandResult {

    //Command executed and handlded
    HANDLED,

    //Show the usage for this command as some parameter is wrong
    SHOW_USAGE,

    //Possibly more?
}

The command context:

package com.gmail.zkfreddit.pluginframework.api.command;

import org.bukkit.command.CommandSender;

import java.util.List;

public interface CommandContext {

    CommandSender getSender();

    List<Object> getArguments();

    @Deprecated
    String getLabel();

    @Deprecated
    //Get the command annotation of the executed command
    Command getCommand();
}

The main command annotation to be put on command methods:

package com.gmail.zkfreddit.pluginframework.api.command;

import org.bukkit.permissions.PermissionDefault;

public @interface Command {

    public static final String DEFAULT_STRING = "";

    String name();

    String description() default DEFAULT_STRING;

    String usageMessage() default DEFAULT_STRING;

    String permission() default DEFAULT_STRING;

    PermissionDefault permissionDefault() default PermissionDefault.TRUE;

    Class[] autoParse() default {};
}

The autoParse intention is that I can define something quick, and if parsing fails, it just displays the usage message of the command.

Now, once I have my implementation written up, I can rewrite the mentioned "sethealth" command executor to something like this:

package com.gmail.zkfreddit.sampleplugin;

import de.web.paulschwandes.pluginframework.api.command.Command;
import de.web.paulschwandes.pluginframework.api.command.CommandContext;
import org.bukkit.entity.Player;
import org.bukkit.permissions.PermissionDefault;

public class BetterCommandExecutor {

    public static enum HealthOperationType {
        CURRENT,
        MAXIMUM;

        public void executeOn(Player player, double newHealth) {
            switch (this) {
                case CURRENT:
                    player.setHealth(newHealth);
                    break;
                case MAXIMUM:
                    player.setMaxHealth(newHealth);
                    break;
            }
        }
    }

    @Command(
            name = "sethealth",
            description = "Set health values for any or all players",
            usageMessage = "/sethealth <current/maximum> <player/* for all> <newHealth>",
            permission = "sampleplugin.sethealth",
            autoParse = {HealthOperationType.class, Player[].class, Double.class} //Player[] as there may be multiple players matched
    )
    public CommandResult setHealth(CommandContext context) {
        HealthOperationType operationType = (HealthOperationType) context.getArguments().get(0);
        Player[] matchedPlayers = (Player[]) context.getArguments().get(1);
        double newHealth = (Double) context.getArguments().get(2);

        for (Player player : matchedPlayers) {
            operationType.executeOn(player, newHealth);
        }

        return CommandResult.HANDLED;
    }
}

I believe I speak for most here that this way feels cleaner.

So where am I asking a question here?

Where I'm stuck.

Child command handling.

In the example, I was able to get away with a simple enum based on the two cases for the first argument.

There may be cases where I have to create a lot of child commands similiar to "current/maximum". A good example may be something that handles joining players together as a team - I would need:

/team create ...
/team delete ...
/team addmember/join ...
/team removemember/leave ...

etc. - I want to be able to create seperate classes for these child commands.

How exactly am I going to introduce a clean way to say "Hey, when the first argument of this matches something, do this and that!" - heck, the "matched" part doesn't even have to be a hardcoded String, I may want something like

/team [player] info

at the same time, while still matching all the previous child commands.

Not only do I have to link to child command methods, I also have to somehow link the required object - after all, my (future) command registration will take an instantiated object (in the example case, of BetterCommandExecutor) and register it. How will I tell "Use this child command instance!" to the registration when passing in the object?

I have been thinking about saying "**** everything, link to a child command class and just instantiate the no-args constructor of it", but while this would probaly procude the least code, it would not give much insight into how exactly child command instances get created. If I do decide to go that way, I'll probaly just define a childs parameter in my Command annotation, and make it take some sort of @ChildCommand annotation list (annotations in annotations? Yo dawk, why not?).

---

So after all this, the question is: With this setup, is there a way I will be able to cleanly define child commands, or will I have to change my footing completely? I thought about extending from some sort of abstract BaseCommand (with an abstract getChildCommands() method), but the annotation method has the advantage of being able to handle multiple commands from one class. Also, as far as I have picked up open-source code until now, I get the impression that extends is 2011 and implements is the flavour of the year, so I should probaly not force myself to extend something every time I'm creating some sort of command handler.

I am sorry for the long post. This went longer than I expected :/

---

Edit #1:

I've just realized what I am basically creating is some sort of...tree? of commands. However, just simply using some sort of CommandTreeBuilder falls away as it goes against one of the things I wanted from this idea: Being able to define multiple command handlers in one class. Back to brainstorming.

Answer 1

The only thing I can think of is splitting your annotations up. You would have one class that has the Base Command as an annotation and then methods in that class with the different sub commands:

@Command("/test")
class TestCommands {

    @Command("sub1"// + more parameters and stuff)
    public Result sub1Command(...) {
        // do stuff
    }

    @Command("sub2"// + more parameters and stuff)
    public Result sub2Command(...) {
        // do stuff
    }
}

If you want more flexibility you could also take the inheritance hierarchy into account, but I'm not sure how self-documenting that would be then (since part of the commands would be hidden away in parent classes).

This solution does not solve your /team [player] info example though, but I think that is a minor thing. It'd be confusing anyway to have subcommands show up in different parameters of your command.

Answer 2

The standard Bukkit API for command handling is pretty good in my opinion, so why not to use it? I think you are just confused, then you avoid it. Here is how I do.

Register the command

Create a new section called commands, where you will put all them as child nodes.

commands:
  sethealth:

Avoid using the permission key: we will check that later. Avoid using the usage key: it is difficult to write a great error message valid in each case. In general, I hate these sub keys, so leave the parent node empty.

Handle it on its own class

Use a separate class which implements the CommandExecutor interface.

public class Sethealth implements CommandExecutor {
    @Override
    public boolean onCommand(CommandSender sender, Command command, String alias, String[] args) {
        // ...
        return true;
    }
}

Add the following under the onEnable() method in the main class.

getCommand("sethealth").setExecutor(new Sethealth());

You do not need to check for command.getName() if you use this class only for this command. Make the method return true in any case: you have not defined the error message, so why should you get it?

Make it safe

You will no longer need to worry about if you process sender at the first line. Also, you may check any generic permissions here.

if (!(sender instanceof Player)) {
    sender.sendMessage("You must be an in-game player.");
    return true;
}
Player player = (Player)sender;
if (!player.hasPermission("sethealth.use")) {
    player.sendMessage(ChatColor.RED + "Insufficient permissions.");
    return true;
}
// ...

You can use colors to make messages more readable.

Dealing with arguments

It is simple to produce 100% reliable results. This is just an incomplete example on how you should work.

if (args.length == 0) {
    player.sendMessage(ChatColor.YELLOW + "Please specify the target.");
    return true;
}
Player target = Server.getPlayer(args[0]);
if (target == null) {
    player.sendMessage(ChatColor.RED + "Target not found.");
    return true;
}
if (args.length == 1) {
    player.sendMessage(ChatColor.YELLOW + "Please specify the new health.");
    return true;
}
try {
    double value = Double.parseDouble(args[1]);
    if (value < 0D || value > 20D) {
        player.sendMessage(ChatColor.RED + "Invalid value.");
        return true;
    }
    target.setHealth(value);
    player.sendMessage(ChatColor.GREEN + target.getName() + "'s health set to " + value + ".");
} catch (NumberFormatException numberFormat) {
    player.sendMessage(ChatColor.RED + "Invalid number.");
}

Plan your code using guard clauses and if you want sub commands, always check them with String.equalsIgnoreCase(String).