Refine CommandDescription builder; add/fix JavaDoc; add tests

- Add tests to check the integrity of the commands that are defined in the CommandManager
- Convert some more commands registrations to the new Builder format

src/main/java/fr/xephi/authme/command/CommandArgumentDescription.java

@@ -15,7 +15,7 @@ public class CommandArgumentDescription {
      */
     private String description;
     /**
-     * Defines whether the argument is isOptional.
+     * Defines whether the argument is optional.
      */
     private boolean isOptional = false;
 
@@ -24,7 +24,7 @@ public class CommandArgumentDescription {
      *
      * @param label       The argument label.
      * @param description The argument description.
-     * @param isOptional  True if the argument is isOptional, false otherwise.
+     * @param isOptional  True if the argument is optional, false otherwise.
      */
     public CommandArgumentDescription(String label, String description, boolean isOptional) {
         this.label = label;

src/main/java/fr/xephi/authme/command/CommandDescription.java

@@ -4,14 +4,25 @@ import fr.xephi.authme.permission.PermissionNode;
 import fr.xephi.authme.util.StringUtils;
 import org.bukkit.command.CommandSender;
 
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
 
 /**
+ * Command description - defines which labels ("names") will lead to a command and points to the
+ * {@link ExecutableCommand} implementation that executes the logic of the command.
+ *
+ * CommandDescription is built hierarchically and have one parent or {@code null} for base commands (main commands
+ * such as /authme) and may have multiple children extending the mapping of the parent: e.g. if /authme has a child
+ * whose label is "register", then "/authme register" is the command that the child defines.
  */
 public class CommandDescription {
 
     /**
-     * Defines the acceptable labels.
+     * Defines the labels to execute the command. For example, if labels are "register" and "r" and the parent is
+     * the command for "/authme", then both "/authme register" and "/authme r" will be handled by this command.
      */
     private List<String> labels;
     /**
@@ -19,7 +30,7 @@ public class CommandDescription {
      */
     private String description;
     /**
-     * Detailed description.
+     * Detailed description of the command.
      */
     private String detailedDescription;
     /**
@@ -31,12 +42,11 @@ public class CommandDescription {
      */
     private CommandDescription parent;
     /**
-     * The child labels.
+     * The child commands that extend this command.
      */
-    // TODO: Remove list instantiation once Builder is the only way to construct objects
     private List<CommandDescription> children = new ArrayList<>();
     /**
-     * The command arguments.
+     * The arguments the command takes.
      */
     private List<CommandArgumentDescription> arguments;
     /**
@@ -124,14 +134,13 @@ public class CommandDescription {
      */
     private CommandDescription(List<String> labels, String description, String detailedDescription,
                                ExecutableCommand executableCommand, CommandDescription parent,
-                               List<CommandDescription> children, List<CommandArgumentDescription> arguments,
+                               List<CommandArgumentDescription> arguments, boolean noArgumentMaximum,
-                               boolean noArgumentMaximum, CommandPermissions permissions) {
+                               CommandPermissions permissions) {
         this.labels = labels;
         this.description = description;
         this.detailedDescription = detailedDescription;
         this.executableCommand = executableCommand;
         this.parent = parent;
-        this.children = children;
         this.arguments = arguments;
         this.noArgumentMaximum = noArgumentMaximum;
         this.permissions = permissions;
@@ -629,7 +638,7 @@ public class CommandDescription {
      * @return Command detailed description.
      */
     public String getDetailedDescription() {
-        return StringUtils.isEmpty(detailedDescription) ? this.detailedDescription : this.description;
+        return !StringUtils.isEmpty(detailedDescription) ? this.detailedDescription : this.description;
     }
 
     /**
@@ -776,8 +785,7 @@ public class CommandDescription {
         private String detailedDescription;
         private ExecutableCommand executableCommand;
         private CommandDescription parent;
-        private List<CommandDescription> children;
+        private List<CommandArgumentDescription> arguments = new ArrayList<>();
-        private List<CommandArgumentDescription> arguments;
         private boolean noArgumentMaximum;
         private CommandPermissions permissions;
 
@@ -793,8 +801,7 @@ public class CommandDescription {
                 firstNonNull(detailedDescription, ""),
                 firstNonNull(executableCommand, null), // TODO ljacqu 20151128: May `executableCommand` be null?
                 firstNonNull(parent, null),
-                valueOrEmptyList(children),
+                arguments,
-                valueOrEmptyList(arguments),
                 noArgumentMaximum,
                 firstNonNull(permissions, null)
             );
@@ -829,19 +836,7 @@ public class CommandDescription {
             return this;
         }
 
-        public Builder children(List<CommandDescription> children) {
-            this.children = children;
-            return this;
-        }
-
-        public Builder children(CommandDescription... children) {
-            return children(asMutableList(children));
-        }
-
         public Builder withArgument(String label, String description, boolean isOptional) {
-            if (arguments == null) {
-                arguments = new ArrayList<>();
-            }
             arguments.add(new CommandArgumentDescription(label, description, isOptional));
             return this;
         }

src/main/java/fr/xephi/authme/command/CommandManager.java

@@ -16,6 +16,7 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 
+import static fr.xephi.authme.command.CommandPermissions.DefaultPermission.ALLOWED;
 import static fr.xephi.authme.command.CommandPermissions.DefaultPermission.OP_ONLY;
 
 /**
@@ -44,6 +45,7 @@ public class CommandManager {
     public void registerCommands() {
         // Create a list of help command labels
         final List<String> helpCommandLabels = Arrays.asList("help", "hlp", "h", "sos", "?");
+        ExecutableCommand helpCommandExecutable = new HelpCommand();
 
         // Register the base AuthMe Reloaded command
         CommandDescription authMeBaseCommand = CommandDescription.builder()
@@ -56,7 +58,7 @@ public class CommandManager {
 
         // Register the help command
         CommandDescription authMeHelpCommand = CommandDescription.builder()
-            .executableCommand(new HelpCommand())
+            .executableCommand(helpCommandExecutable)
             .labels(helpCommandLabels)
             .description("View help")
             .detailedDescription("View detailed help pages about AuthMeReloaded commands.")
@@ -88,51 +90,49 @@ public class CommandManager {
             .build();
 
         // Register the forcelogin command
-        CommandDescription forceLoginCommand = new CommandDescription(new ForceLoginCommand(), new ArrayList<String>() {
+        CommandDescription forceLoginCommand = CommandDescription.builder()
-
+            .executableCommand(new ForceLoginCommand())
-            {
+            .labels("forcelogin", "login")
-                add("forcelogin");
+            .description("Enforce login player")
-                add("login");
+            .detailedDescription("Enforce the specified player to login.")
-            }
+            .parent(authMeBaseCommand)
-        }, "Enforce login player", "Enforce the specified player to login.", authMeBaseCommand);
+            .permissions(OP_ONLY, UserPermission.CAN_LOGIN_BE_FORCED)
-        forceLoginCommand.setCommandPermissions(UserPermission.CAN_LOGIN_BE_FORCED, OP_ONLY);
+            .withArgument("player", "Online player name", true)
-        forceLoginCommand.addArgument(new CommandArgumentDescription("player", "Online player name", true));
+            .build();
 
         // Register the changepassword command
-        CommandDescription changePasswordCommand = new CommandDescription(new ChangePasswordCommand(), new ArrayList<String>() {
+        CommandDescription changePasswordCommand = CommandDescription.builder()
+            .executableCommand(new ChangePasswordCommand())
+            .labels("password", "changepassword", "changepass", "cp")
+            .description("Change a player's password")
+            .detailedDescription("Change the password of a player.")
+            .parent(authMeBaseCommand)
+            .permissions(OP_ONLY, UserPermission.CHANGE_PASSWORD)
+            .withArgument("player", "Player name", false)
+            .withArgument("pwd", "New password", false)
+            .build();
 
-            {
+        // Register the last login command
-                add("password");
+        CommandDescription lastLoginCommand = CommandDescription.builder()
-                add("changepassword");
+            .executableCommand(new LastLoginCommand())
-                add("changepass");
+            .labels("lastlogin", "ll")
-                add("cp");
+            .description("Player's last login")
-            }
+            .detailedDescription("View the date of the specified players last login")
-        }, "Change player's password", "Change the password of a player.", authMeBaseCommand);
+            .parent(authMeBaseCommand)
-        changePasswordCommand.setCommandPermissions(AdminPermission.CHANGE_PASSWORD, OP_ONLY);
+            .permissions(OP_ONLY, AdminPermission.LAST_LOGIN)
-        changePasswordCommand.addArgument(new CommandArgumentDescription("player", "Player name", false));
+            .withArgument("player", "Player name", true)
-        changePasswordCommand.addArgument(new CommandArgumentDescription("pwd", "New password", false));
+            .build();
-
-        // Register the purge command
-        CommandDescription lastLoginCommand = new CommandDescription(new LastLoginCommand(), new ArrayList<String>() {
-
-            {
-                add("lastlogin");
-                add("ll");
-            }
-        }, "Player's last login", "View the date of the specified players last login", authMeBaseCommand);
-        lastLoginCommand.setCommandPermissions(AdminPermission.LAST_LOGIN, OP_ONLY);
-        lastLoginCommand.addArgument(new CommandArgumentDescription("player", "Player name", true));
 
         // Register the accounts command
-        CommandDescription accountsCommand = new CommandDescription(new AccountsCommand(), new ArrayList<String>() {
+        CommandDescription accountsCommand = CommandDescription.builder()
-
+            .executableCommand(new AccountsCommand())
-            {
+            .labels("accounts", "account")
-                add("accounts");
+            .description("Display player accounts")
-                add("account");
+            .detailedDescription("Display all accounts of a player by his player name or IP.")
-            }
+            .parent(authMeBaseCommand)
-        }, "Display player accounts", "Display all accounts of a player by it's player name or IP.", authMeBaseCommand);
+            .permissions(OP_ONLY, AdminPermission.ACCOUNTS)
-        accountsCommand.setCommandPermissions(AdminPermission.ACCOUNTS, OP_ONLY);
+            .withArgument("player", "Player name or IP", true)
-        accountsCommand.addArgument(new CommandArgumentDescription("player", "Player name or IP", true));
+            .build();
 
         // Register the getemail command
         CommandDescription getEmailCommand = new CommandDescription(new GetEmailCommand(), new ArrayList<String>() {
@@ -286,28 +286,28 @@ public class CommandManager {
         reloadCommand.setCommandPermissions(AdminPermission.RELOAD, OP_ONLY);
 
         // Register the version command
-        /* TODO ljacqu 20151128: labels should not be helpCommandLabels! Previously it was:
+        CommandDescription versionCommand = CommandDescription.builder()
-        add("version");
+            .executableCommand(new VersionCommand())
-        add("ver");
+            .labels("version", "ver", "v", "about", "info")
-        add("v");
+            .description("Version info")
-        add("about");
+            .detailedDescription("Show detailed information about the installed AuthMeReloaded version, and shows the "
-        add("info");*/
+                + "developers, contributors, license and other information.")
-        CommandDescription versionCommand = new CommandDescription(new VersionCommand(), helpCommandLabels,
+            .parent(authMeBaseCommand)
-            "Version info", "Show detailed information about the installed AuthMeReloaded version, and shows the developers, contributors, license and other information.", authMeBaseCommand);
+            .build();
 
         // Register the base login command
-        CommandDescription loginBaseCommand = new CommandDescription(new LoginCommand(), new ArrayList<String>() {
+        CommandDescription loginBaseCommand = CommandDescription.builder()
-
+            .executableCommand(new LoginCommand())
-            {
+            .labels("login", "l")
-                add("login");
+            .description("Login command")
-                add("l");
+            .detailedDescription("Command to log in using AuthMeReloaded.")
-            }
+            .parent(null)
-        }, "Login command", "Command to login using AuthMeReloaded.", null);
+            .permissions(ALLOWED, UserPermission.LOGIN)
-        loginBaseCommand.setCommandPermissions(UserPermission.LOGIN, CommandPermissions.DefaultPermission.ALLOWED);
+            .withArgument("password", "Login password", false)
-        loginBaseCommand.addArgument(new CommandArgumentDescription("password", "Login password", false));
+            .build();
 
         // Register the help command
-        CommandDescription loginHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels,
+        CommandDescription loginHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels,
             "View help", "View detailed help pages about AuthMeReloaded login commands.", loginBaseCommand);
         loginHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true));
 
@@ -321,7 +321,7 @@ public class CommandManager {
         logoutBaseCommand.setCommandPermissions(UserPermission.LOGOUT, CommandPermissions.DefaultPermission.ALLOWED);
 
         // Register the help command
-        CommandDescription logoutHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels,
+        CommandDescription logoutHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels,
             "View help", "View detailed help pages about AuthMeReloaded logout commands.", logoutBaseCommand);
         logoutHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true));
 
@@ -338,7 +338,7 @@ public class CommandManager {
         registerBaseCommand.addArgument(new CommandArgumentDescription("verifyPassword", "Verify password", false));
 
         // Register the help command
-        CommandDescription registerHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels,
+        CommandDescription registerHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels,
             "View help", "View detailed help pages about AuthMeReloaded register commands.", registerBaseCommand);
         registerHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true));
 
@@ -354,7 +354,7 @@ public class CommandManager {
         unregisterBaseCommand.addArgument(new CommandArgumentDescription("password", "Password", false));
 
         // Register the help command
-        CommandDescription unregisterHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels, "View help", "View detailed help pages about AuthMeReloaded unregister commands.", unregisterBaseCommand);
+        CommandDescription unregisterHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels, "View help", "View detailed help pages about AuthMeReloaded unregister commands.", unregisterBaseCommand);
         unregisterHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true));
 
         // Register the base changepassword command
@@ -370,12 +370,12 @@ public class CommandManager {
         changePasswordBaseCommand.addArgument(new CommandArgumentDescription("verifyPassword", "Verify password", false));
 
         // Register the help command
-        CommandDescription changePasswordHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels,
+        CommandDescription changePasswordHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels,
             "View help", "View detailed help pages about AuthMeReloaded change password commands.", changePasswordBaseCommand);
         changePasswordHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true));
 
         // Register the base Dungeon Maze command
-        CommandDescription emailBaseCommand = new CommandDescription(new HelpCommand(), new ArrayList<String>() {
+        CommandDescription emailBaseCommand = new CommandDescription(helpCommandExecutable, new ArrayList<String>() {
 
             {
                 add("email");
@@ -384,7 +384,7 @@ public class CommandManager {
         }, "E-mail command", "The AuthMe Reloaded E-mail command. The root for all E-mail commands.", null);
 
         // Register the help command
-        CommandDescription emailHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels,
+        CommandDescription emailHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels,
             "View help", "View detailed help pages about AuthMeReloaded help commands.", emailBaseCommand);
         emailHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true));
 
@@ -439,7 +439,7 @@ public class CommandManager {
         captchaBaseCommand.addArgument(new CommandArgumentDescription("captcha", "The captcha", false));
 
         // Register the help command
-        CommandDescription captchaHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels,
+        CommandDescription captchaHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels,
             "View help", "View detailed help pages about AuthMeReloaded change captcha commands.", captchaBaseCommand);
         captchaHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true));
 
@@ -456,7 +456,7 @@ public class CommandManager {
         converterBaseCommand.addArgument(new CommandArgumentDescription("job", "Conversion job: flattosql / flattosqlite /| xauth / crazylogin / rakamak / royalauth / vauth / sqltoflat", false));
 
         // Register the help command
-        CommandDescription converterHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels,
+        CommandDescription converterHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels,
             "View help", "View detailed help pages about AuthMeReloaded change captcha commands.", converterBaseCommand);
         converterHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true));
 

src/test/java/fr/xephi/authme/command/CommandManagerTest.java

@@ -1,10 +1,18 @@
 package fr.xephi.authme.command;
 
+import fr.xephi.authme.util.StringUtils;
+import org.junit.Ignore;
 import org.junit.Test;
 
+import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
+import java.util.Set;
 
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assert.assertThat;
 
 /**
@@ -12,6 +20,12 @@ import static org.junit.Assert.assertThat;
  */
 public class CommandManagerTest {
 
+    /**
+     * Defines the maximum allowed depths for nesting CommandDescription instances.
+     * Note that the depth starts at 0 (e.g. /authme), so a depth of 2 is something like /authme hello world
+     */
+    private static int MAX_ALLOWED_DEPTH = 2;
+
     @Test
     public void shouldInitializeCommands() {
         // given/when
@@ -29,6 +43,108 @@ public class CommandManagerTest {
         assertThat(commandsIncludeLabel(commands, "help"), equalTo(false));
     }
 
+    @Test
+    public void shouldNotBeNestedExcessively() {
+        // given
+        CommandManager manager = new CommandManager(true);
+        Consumer descriptionTester = new Consumer() {
+            @Override
+            public void accept(CommandDescription command, int depth) {
+                assertThat(depth <= MAX_ALLOWED_DEPTH, equalTo(true));
+            }
+        };
+
+        // when/then
+        List<CommandDescription> commands = manager.getCommandDescriptions();
+        walkThroughCommands(commands, descriptionTester);
+    }
+
+    @Test
+    @Ignore
+    public void shouldNotDefineSameLabelTwice() {
+        // given
+        CommandManager manager = new CommandManager(true);
+        Set<String> commandMappings = new HashSet<>();
+        Consumer uniqueMappingTester = new Consumer() {
+            @Override
+            public void accept(CommandDescription command, int depth) {
+
+            }
+        };
+
+        // when
+        // TODO finish this
+    }
+
+    /**
+     * The description should provide a very short description of the command and shouldn't end in a ".", whereas the
+     * detailed description should be longer and end with a period.
+     */
+    @Test
+    public void shouldHaveDescription() {
+        // given
+        CommandManager manager = new CommandManager(true);
+        Consumer descriptionTester = new Consumer() {
+            @Override
+            public void accept(CommandDescription command, int depth) {
+                assertThat("has description", StringUtils.isEmpty(command.getDescription()), equalTo(false));
+                assertThat("short description doesn't end in '.'", command.getDescription().endsWith("."),
+                    equalTo(false));
+                assertThat("has detailed description", StringUtils.isEmpty(command.getDetailedDescription()),
+                    equalTo(false));
+                assertThat("detailed description ends in '.'", command.getDetailedDescription().endsWith("."),
+                    equalTo(true));
+            }
+        };
+
+        // when
+        List<CommandDescription> commands = manager.getCommandDescriptions();
+        walkThroughCommands(commands, descriptionTester);
+    }
+
+    @Test
+    public void shouldNotHaveMultipleInstancesOfSameExecutableCommandSubType() {
+        // given
+        final Map<Class<? extends ExecutableCommand>, ExecutableCommand> implementations = new HashMap<>();
+        CommandManager manager = new CommandManager(true);
+        Consumer descriptionTester = new Consumer() {
+            @Override
+            public void accept(CommandDescription command, int depth) {
+                assertThat(command.getExecutableCommand(), not(nullValue()));
+                ExecutableCommand commandExec = command.getExecutableCommand();
+                ExecutableCommand storedExec = implementations.get(command.getExecutableCommand().getClass());
+                if (storedExec != null) {
+                    assertThat("has same implementation of '" + storedExec.getClass().getName() + "' for command with "
+                        + "parent " + (command.getParent() == null ? "null" : command.getParent().getLabels()),
+                        storedExec == commandExec, equalTo(true));
+                } else {
+                    implementations.put(commandExec.getClass(), commandExec);
+                }
+            }
+        };
+
+        // when
+        List<CommandDescription> commands = manager.getCommandDescriptions();
+        walkThroughCommands(commands, descriptionTester);
+    }
+
+
+    // ------------
+    // Helper methods
+    // ------------
+    private static void walkThroughCommands(List<CommandDescription> commands, Consumer consumer) {
+        walkThroughCommands(commands, consumer, 0);
+    }
+
+    private static void walkThroughCommands(List<CommandDescription> commands, Consumer consumer, int depth) {
+        for (CommandDescription command : commands) {
+            consumer.accept(command, depth);
+            if (command.hasChildren()) {
+                walkThroughCommands(command.getChildren(), consumer, depth + 1);
+            }
+        }
+    }
+
     private static boolean commandsIncludeLabel(Iterable<CommandDescription> commands, String label) {
         for (CommandDescription command : commands) {
             if (command.getLabels().contains(label)) {
@@ -38,4 +154,8 @@ public class CommandManagerTest {
         return false;
     }
 
+    private interface Consumer {
+        void accept(CommandDescription command, int depth);
+    }
+
 }