Base Class System for splitting a spigot command in subcommands

Question

Related to this question: Spigot Plugin: Generic form of the plugin's main clas

I've created a Clan-Plugin, in which I have one "main" command, which is simply /clan. Then there are several sub-commands, e.g. /clan leave, /clan money, etc. There are also subcommands that require multiple arguments, like /clan create, where you have to provide details about the clan you want to create.

My very basic problem is, that spigot only offers the possibility to implement commands based on the first word, and not the arguments. What you have to do is manually differ between the subcommands, and then execute the code. In the past I did this by having a massive if-elseif-elseif-... construct in the executor method of the command, with the code of the sub-commands being placed in methods. However, that made this class become really massive oover the time, until it hit the 1000 lines recently. I really thought I should refactor the command, so I came up with the following idea (which I successfully implemented).

I created a base class for all subcommands, AbstractCommand, and a Child Class (which is still abstract) for sub-commands, that have to be confirmed before being executed (e.g. deletion of the clan) AbstractConfirmCommand. Also I wrote a little CommandRegistry-Class to store all the implementations of the AbstractCommand, and find the proper one to execute when necessary. Then in my Main class (which can be found in above link, if there is anyone interested), I register all the Implementations of AbstractCommand. My "Spigot-ClanCommand-Class" has now shrunk down to 80 lines, with which I'm quite happy to be honest. However, I'm not experienced at all with abstract classes, and am not even sure if an abstract class was the better choice over an interface. Here's my code, I hope I could make it clear what it's supposed to do.

AbstractCommand:

import org.bukkit.command.Command;
import org.bukkit.entity.Player;

public abstract class AbstractCommand {

    protected final String commandName;
    
    protected AbstractCommand(String commandName) {
        this.commandName = commandName;
    }
    
    public abstract void execute(Player player, Command cmd, String arg2, String[] args);
    
    public String getCommandName() {
        return commandName;
    }
    
}

AbstractConfirmCommand:

import org.bukkit.command.Command;
import org.bukkit.entity.Player;

import com.clanplugin.manager.MessageManager;

public abstract class AbstractConfirmCommand extends AbstractCommand {

    private int requiredPositionOfConfirm = 1;
    
    protected AbstractConfirmCommand(String commandName) {
        super(commandName);
    }

    protected void setConfirmPosition(int position) {
        requiredPositionOfConfirm = position;
    }
    
    @Override
    public void execute(Player player, Command cmd, String arg2, String[] args) {
        if (args.length < requiredPositionOfConfirm || args.length > requiredPositionOfConfirm + 1) {
            player.sendMessage(MessageManager.badNumberOfArguments());
            return;
        }
        
        if (args.length == requiredPositionOfConfirm) {
            withoutConfirm(player, cmd, arg2, args);
            return;
        }
        
        if (args.length == requiredPositionOfConfirm + 1) {
            if (args[requiredPositionOfConfirm].equalsIgnoreCase("confirm")) {
                withConfirm(player, cmd, arg2, args);
            } else {
                withoutConfirm(player, cmd, arg2, args);
            }
            return;
        }
    }
    
    protected abstract void withoutConfirm(Player player, Command cmd, String arg2, String[] args);
    
    protected abstract void withConfirm(Player player, Command cmd, String arg2, String[] args);
}

CommandRegistry:

import java.util.HashSet;

import org.bukkit.command.Command;
import org.bukkit.entity.Player;

import com.clansystem.manager.MessageManager;

public class CommandRegistry {

    private HashSet<AbstractCommand> registeredCommands;
    
    public CommandRegistry() {
        registeredCommands = new HashSet<AbstractCommand>();
    }
    
    public void registerCommand(AbstractCommand command) {
        registeredCommands.add(command);
    }
    
    public void executeCommand(Player player, Command cmd, String arg2, String[] args) {
        for (AbstractCommand registeredCommand : registeredCommands) {
            if (registeredCommand.getCommandName().equalsIgnoreCase(args[0])) {
                registeredCommand.execute(player, cmd, arg2, args);
                return;
            }
        }
        player.sendMessage(MessageManager.getHelpMessage());
    }
}

ClanCommand:

public class ClanCommand implements CommandExecutor {
    @Override
    public boolean onCommand(CommandSender sender, Command cmd, String arg2, String[] args) {

        if (!(sender instanceof Player)) {
            sender.sendMessage("Clan-Commands können nur von Spielern ausgeführt werden.");
            return true;
        }

        Player player = (Player) sender;

        //Some checks which are irrelevant here... (e.g. command cooldown, permission-check etc)
        
        if (args.length == 0) {
            player.sendMessage(MessageManager.getHelpMessage());
            return true;
        }

        
        Main.getCommandRegistry().executeCommand(player, cmd, arg2, args);

        return true;
    }
}

Finally, I'll append an example of an implementation of AbstractCommand:

import org.bukkit.command.Command;
import org.bukkit.entity.Player;

import com.clanplugin.commands.AbstractCommand;
import com.clanplugin.manager.MessageManager;
import com.clanplugin.utils.PermissionUtils;

public class ShowMaxClanMemberCommand extends AbstractCommand {

    public ShowMaxClanMemberCommand() {
        super("maxmember");
    }

    @Override
    public void execute(Player player, Command cmd, String arg2, String[] args) {
        int limit = PermissionUtils.getTotalClanMembersAllowed(player);

        player.sendMessage(MessageManager.getMaxMemberMessage(limit));
    }

}

What I want to know is, if the basic idea of creating an abstract class is "good practice", and how I can improve my construct. Also I'm pretty new to the site, and I'm not sure if this is too much code for one post. If so, please tell me :)

Answer 1

Nice implementation and refactoring, few suggestions:

AbstractCommand class

I created a base class for all subcommands, AbstractCommand.

• I think a better name is SubCommand. The type of the class doesn't need to be in the name.

• The instance variable commandName can be shorten to name, since it's implicit that it's the subcommand name.

• The second argument of the method execute is called arg2, a better name could be label.

• The method execute returns nothing. How to know the result of the command? How to handle errors and exceptions? Consider to return at least a boolean.

AbstractConfirmCommand class

This class seems to check if the confirm argument is where is supposed to be and then invoke withConfirm or withoutConfirm accordingly. There is no example of a subclass so I'll limit the review to what I see:

• The chain of if()-return should be replaced with if()-else if()-else. No need for the empty return.
• A better class name could be ConfirmedCommand
• Pushing common behavior up to a parent class is ok, but I am not sure if splitting the method execute to withConfirm and withoutConfirm is a good idea. You are basically forcing all subclasses to implement two methods instead of one.

CommandRegistry class

• If it's a registry, it shouldn't execute the commands. That is the job of the CommandExecutor.
• Instead of a Set consider using a Map<String,SubCommand>, it's way more efficient for retrieving data.

I suggest to add a method addCommand to ClanCommand (which is your CommandExecutor) and store the subcommands in a map.

ClanCommand class

• Since it implements CommandExecutor, I would call it ClanCommandExecutor.
• Same as before, instead of a chain of if()-return true-if()-return true consider to find the conditions to fail first with if this than return false, then use if-else if-else, and finally return true.

There is a design pattern called Command which I suggest you to check out.