#305 Adjust behavior of label handling; cleanup

- Fix bugs in behavior (wrong labels being shown for help)
- Change order of labels and arguments in FoundCommandResult constructors
- Move FoundResultStatus enum to its own class
- Create test class for HelpProvider

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

@@ -12,9 +12,9 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.Set;
 
-import static fr.xephi.authme.command.FoundCommandResult.ResultStatus.INCORRECT_ARGUMENTS;
+import static fr.xephi.authme.command.FoundResultStatus.INCORRECT_ARGUMENTS;
-import static fr.xephi.authme.command.FoundCommandResult.ResultStatus.MISSING_BASE_COMMAND;
+import static fr.xephi.authme.command.FoundResultStatus.MISSING_BASE_COMMAND;
-import static fr.xephi.authme.command.FoundCommandResult.ResultStatus.UNKNOWN_LABEL;
+import static fr.xephi.authme.command.FoundResultStatus.UNKNOWN_LABEL;
 
 /**
  * The AuthMe command handler, responsible for mapping incoming commands to the correct {@link CommandDescription}
@@ -121,9 +121,8 @@ public class CommandHandler {
 
         // Show a command suggestion if available and the difference isn't too big
         if (result.getDifference() < SUGGEST_COMMAND_THRESHOLD && result.getCommandDescription() != null) {
-            sender.sendMessage(ChatColor.YELLOW + "Did you mean " + ChatColor.GOLD + "/"
+            sender.sendMessage(ChatColor.YELLOW + "Did you mean " + ChatColor.GOLD
-                + result.getCommandDescription() + ChatColor.YELLOW + "?");
+                + CommandUtils.constructCommandPath(result.getCommandDescription()) + ChatColor.YELLOW + "?");
-            // FIXME: Define a proper string representation of command description
         }
 
         sender.sendMessage(ChatColor.YELLOW + "Use the command " + ChatColor.GOLD + "/" + result.getLabels().get(0)
@@ -175,9 +174,9 @@ public class CommandHandler {
         List<String> remainingParts = parts.subList(1, parts.size());
         CommandDescription childCommand = getSuitableChild(base, remainingParts);
         if (childCommand != null) {
-            return new FoundCommandResult(childCommand, parts.subList(2, parts.size()), parts.subList(0, 2));
+            return new FoundCommandResult(childCommand, parts.subList(0, 2), parts.subList(2, parts.size()));
         } else if (hasSuitableArgumentCount(base, remainingParts.size())) {
-            return new FoundCommandResult(base, parts.subList(1, parts.size()), parts.subList(0, 1));
+            return new FoundCommandResult(base, parts.subList(0, 1), parts.subList(1, parts.size()));
         }
 
         return getCommandWithSmallestDifference(base, parts);
@@ -200,13 +199,15 @@ public class CommandHandler {
 
         // base command may have no children or no child label was present
         if (closestCommand == null) {
-            return new FoundCommandResult(null, null, parts, minDifference, UNKNOWN_LABEL);
+            return new FoundCommandResult(null, parts, null, minDifference, UNKNOWN_LABEL);
         }
 
-        FoundCommandResult.ResultStatus status = (minDifference == 0.0) ? INCORRECT_ARGUMENTS : UNKNOWN_LABEL;
+        FoundResultStatus status = (minDifference == 0.0) ? INCORRECT_ARGUMENTS : UNKNOWN_LABEL;
         final int partsSize = parts.size();
         List<String> labels = parts.subList(0, Math.min(closestCommand.getParentCount() + 1, partsSize));
-        List<String> arguments = parts.subList(Math.min(partsSize - 1, labels.size()), partsSize);
+        List<String> arguments = (labels.size() == partsSize)
+            ? new ArrayList<String>()
+            : parts.subList(labels.size(), partsSize);
 
         return new FoundCommandResult(closestCommand, labels, arguments, minDifference, status);
     }

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

@@ -1,7 +1,6 @@
 package fr.xephi.authme.command;
 
 import com.google.common.collect.Lists;
-import fr.xephi.authme.util.CollectionUtils;
 import fr.xephi.authme.util.StringUtils;
 
 import java.util.ArrayList;
@@ -25,7 +24,7 @@ public final class CommandUtils {
 
     /**
      * Provide a textual representation of a list of labels to show it as a command. For example, a list containing
-     * the items ["authme", "register", "player"] it will return "authme register player".
+     * the items ["authme", "register", "player"] will return "authme register player".
      *
      * @param labels The labels to format
      *

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

@@ -4,17 +4,17 @@ import java.util.List;
 
 /**
  * Result of a command mapping by {@link CommandHandler}. An object of this class represents a successful mapping
- * as well as erroneous ones, as communicated with {@link ResultStatus}.
+ * as well as erroneous ones, as communicated with {@link FoundResultStatus}.
  * <p />
- * Fields other than {@link ResultStatus} are available depending, among other factors, on the status:
+ * Fields other than {@link FoundResultStatus} are available depending, among other factors, on the status:
  * <ul>
- *   <li>{@link ResultStatus#SUCCESS} entails that mapping the input to a command was successful. Therefore,
+ *   <li>{@link FoundResultStatus#SUCCESS} entails that mapping the input to a command was successful. Therefore,
  *       the command description, labels and arguments are set. The difference is 0.0.</li>
- *   <li>{@link ResultStatus#INCORRECT_ARGUMENTS}: The received parts could be mapped to a command but the argument
+ *   <li>{@link FoundResultStatus#INCORRECT_ARGUMENTS}: The received parts could be mapped to a command but the argument
  *       count doesn't match. Guarantees that the command description field is not null; difference is 0.0</li>
- *   <li>{@link ResultStatus#UNKNOWN_LABEL}: The labels could not be mapped to a command. The command description may
+ *   <li>{@link FoundResultStatus#UNKNOWN_LABEL}: The labels could not be mapped to a command. The command description
- *       be set to the most similar command, or it may be null. Difference is above 0.0.</li>
+ *       may be set to the most similar command, or it may be null. Difference is above 0.0.</li>
- *   <li>{@link ResultStatus#MISSING_BASE_COMMAND} should never occur. All other fields may be null and any further
+ *   <li>{@link FoundResultStatus#MISSING_BASE_COMMAND} should never occur. All other fields may be null and any further
  *       processing of the object should be aborted.</li>
  * </ul>
  */
@@ -24,38 +24,38 @@ public class FoundCommandResult {
      * The command description instance.
      */
     private final CommandDescription commandDescription;
-    /**
-     * The command arguments.
-     */
-    private final List<String> arguments;
     /**
      * The labels used to invoke the command. This may be different for the same {@link ExecutableCommand} instance
      * if multiple labels have been defined, e.g. "/authme register" and "/authme reg".
      */
     private final List<String> labels;
+    /**
+     * The command arguments.
+     */
+    private final List<String> arguments;
 
     private final double difference;
 
-    private final ResultStatus resultStatus;
+    private final FoundResultStatus resultStatus;
 
     /**
      * Constructor.
      *
      * @param commandDescription The command description.
+     * @param labels             The labels used to access the command.
      * @param arguments          The command arguments.
-     * @param labels             The original query reference.
      */
-    public FoundCommandResult(CommandDescription commandDescription, List<String> arguments, List<String> labels,
+    public FoundCommandResult(CommandDescription commandDescription, List<String> labels, List<String> arguments,
-                              double difference, ResultStatus resultStatus) {
+                              double difference, FoundResultStatus resultStatus) {
         this.commandDescription = commandDescription;
-        this.arguments = arguments;
         this.labels = labels;
+        this.arguments = arguments;
         this.difference = difference;
         this.resultStatus = resultStatus;
     }
 
-    public FoundCommandResult(CommandDescription commandDescription, List<String> arguments, List<String> labels) {
+    public FoundCommandResult(CommandDescription commandDescription, List<String> labels, List<String> arguments) {
-        this(commandDescription, arguments, labels, 0.0, ResultStatus.SUCCESS);
+        this(commandDescription, labels, arguments, 0.0, FoundResultStatus.SUCCESS);
     }
 
     public CommandDescription getCommandDescription() {
@@ -74,18 +74,8 @@ public class FoundCommandResult {
         return difference;
     }
 
-    public ResultStatus getResultStatus() {
+    public FoundResultStatus getResultStatus() {
         return resultStatus;
     }
 
-    public enum ResultStatus {
-
-        SUCCESS,
-
-        INCORRECT_ARGUMENTS,
-
-        UNKNOWN_LABEL,
-
-        MISSING_BASE_COMMAND
-    }
 }

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

@@ -0,0 +1,16 @@
+package fr.xephi.authme.command;
+
+/**
+ * Result status for mapping command parts. See {@link FoundCommandResult} for a detailed description of the states.
+ */
+public enum FoundResultStatus {
+
+    SUCCESS,
+
+    INCORRECT_ARGUMENTS,
+
+    UNKNOWN_LABEL,
+
+    MISSING_BASE_COMMAND
+
+}

src/main/java/fr/xephi/authme/command/executable/HelpCommand.java

@@ -5,6 +5,7 @@ import fr.xephi.authme.command.CommandHandler;
 import fr.xephi.authme.command.CommandUtils;
 import fr.xephi.authme.command.ExecutableCommand;
 import fr.xephi.authme.command.FoundCommandResult;
+import fr.xephi.authme.command.FoundResultStatus;
 import fr.xephi.authme.command.help.HelpProvider;
 import fr.xephi.authme.permission.PermissionsManager;
 import org.bukkit.ChatColor;
@@ -12,7 +13,9 @@ import org.bukkit.command.CommandSender;
 
 import java.util.List;
 
-import static fr.xephi.authme.command.FoundCommandResult.ResultStatus.*;
+import static fr.xephi.authme.command.FoundResultStatus.INCORRECT_ARGUMENTS;
+import static fr.xephi.authme.command.FoundResultStatus.MISSING_BASE_COMMAND;
+import static fr.xephi.authme.command.FoundResultStatus.UNKNOWN_LABEL;
 
 public class HelpCommand extends ExecutableCommand {
 
@@ -27,8 +30,9 @@ public class HelpCommand extends ExecutableCommand {
         // TODO ljacqu 20151213: This is essentially the same logic as in CommandHandler and we'd like to have the same
         // messages. Maybe we can have another method in CommandHandler where the end command isn't executed upon
         // success.
-        FoundCommandResult.ResultStatus resultStatus = foundCommandResult.getResultStatus();
+        FoundResultStatus resultStatus = foundCommandResult.getResultStatus();
         if (MISSING_BASE_COMMAND.equals(resultStatus)) {
+            // FIXME something wrong - this error appears
             sender.sendMessage(ChatColor.DARK_RED + "Could not get base command");
             return;
         } else if (INCORRECT_ARGUMENTS.equals(resultStatus) || UNKNOWN_LABEL.equals(resultStatus)) {

src/main/java/fr/xephi/authme/command/help/HelpProvider.java

@@ -1,5 +1,6 @@
 package fr.xephi.authme.command.help;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import fr.xephi.authme.command.CommandArgumentDescription;
@@ -216,11 +217,12 @@ public final class HelpProvider {
         return (flag & options) != 0;
     }
 
-    private static List<String> filterCorrectLabels(CommandDescription command, List<String> labels) {
+    @VisibleForTesting
+    protected static List<String> filterCorrectLabels(CommandDescription command, List<String> labels) {
         List<CommandDescription> commands = new ArrayList<>(command.getParentCount() + 1);
         CommandDescription currentCommand = command;
         while (currentCommand != null) {
-            commands.add(command);
+            commands.add(currentCommand);
             currentCommand = currentCommand.getParent();
         }
         commands = Lists.reverse(commands);

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

@@ -53,7 +53,7 @@ public class CommandHandlerTest {
 
         // then
         assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("login", "authme")));
-        assertThat(result.getResultStatus(), equalTo(FoundCommandResult.ResultStatus.SUCCESS));
+        assertThat(result.getResultStatus(), equalTo(FoundResultStatus.SUCCESS));
         assertThat(result.getArguments(), contains("test1"));
         assertThat(result.getDifference(), equalTo(0.0));
     }
@@ -68,7 +68,7 @@ public class CommandHandlerTest {
 
         // then
         assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("register", "authme")));
-        assertThat(result.getResultStatus(), equalTo(FoundCommandResult.ResultStatus.SUCCESS));
+        assertThat(result.getResultStatus(), equalTo(FoundResultStatus.SUCCESS));
         assertThat(result.getArguments(), contains("arg1", "arg2"));
         assertThat(result.getDifference(), equalTo(0.0));
     }
@@ -83,7 +83,21 @@ public class CommandHandlerTest {
 
         // then
         assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("register", "authme")));
-        assertThat(result.getResultStatus(), equalTo(FoundCommandResult.ResultStatus.INCORRECT_ARGUMENTS));
+        assertThat(result.getResultStatus(), equalTo(FoundResultStatus.INCORRECT_ARGUMENTS));
+        assertThat(result.getDifference(), equalTo(0.0));
+    }
+
+    @Test
+    public void shouldRejectCommandWithTooFewArguments() {
+        // given
+        List<String> parts = Arrays.asList("authme", "Reg");
+
+        // when
+        FoundCommandResult result = handler.mapPartsToCommand(parts);
+
+        // then
+        assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("register", "authme")));
+        assertThat(result.getResultStatus(), equalTo(FoundResultStatus.INCORRECT_ARGUMENTS));
         assertThat(result.getDifference(), equalTo(0.0));
     }
 
@@ -97,7 +111,7 @@ public class CommandHandlerTest {
 
         // then
         assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("register", "authme")));
-        assertThat(result.getResultStatus(), equalTo(FoundCommandResult.ResultStatus.UNKNOWN_LABEL));
+        assertThat(result.getResultStatus(), equalTo(FoundResultStatus.UNKNOWN_LABEL));
         assertThat(result.getDifference() < 0.75, equalTo(true));
     }
 
@@ -112,7 +126,7 @@ public class CommandHandlerTest {
 
         // then
         assertThat(result.getCommandDescription(), not(nullValue()));
-        assertThat(result.getResultStatus(), equalTo(FoundCommandResult.ResultStatus.UNKNOWN_LABEL));
+        assertThat(result.getResultStatus(), equalTo(FoundResultStatus.UNKNOWN_LABEL));
         assertThat(result.getDifference() > 0.75, equalTo(true));
     }
 

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

@@ -8,6 +8,7 @@ import java.util.List;
 
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertThat;
+import static org.mockito.Mockito.mock;
 
 /**
  * Test for {@link CommandUtils}.
@@ -37,4 +38,92 @@ public class CommandUtilsTest {
         // then
         assertThat(str, equalTo(""));
     }
+
+    @Test
+    public void shouldPrintLabels() {
+        // given
+        List<String> labels = Arrays.asList("authme", "help", "reload");
+
+        // when
+        String result = CommandUtils.labelsToString(labels);
+
+        // then
+        assertThat(result, equalTo("authme help reload"));
+    }
+
+    @Test
+    public void shouldReturnCommandPath() {
+        // given
+        CommandDescription base = CommandDescription.builder()
+            .labels("authme", "auth")
+            .description("Base")
+            .detailedDescription("Test base command.")
+            .executableCommand(mock(ExecutableCommand.class))
+            .build();
+        CommandDescription command = CommandDescription.builder()
+            .parent(base)
+            .labels("help", "h", "?")
+            .description("Child")
+            .detailedDescription("Test child command.")
+            .executableCommand(mock(ExecutableCommand.class))
+            .build();
+
+        // when
+        String commandPath = CommandUtils.constructCommandPath(command);
+
+        // then
+        assertThat(commandPath, equalTo("/authme help"));
+    }
+
+
+    // ------
+    // min / max arguments
+    // ------
+    @Test
+    public void shouldComputeMinAndMaxOnEmptyCommand() {
+        // given
+        CommandDescription command = getBuilderForArgsTest().build();
+
+        // when / then
+        checkArgumentCount(command, 0, 0);
+    }
+
+    @Test
+    public void shouldComputeMinAndMaxOnCommandWithMandatoryArgs() {
+        // given
+        CommandDescription command = getBuilderForArgsTest()
+            .withArgument("Test", "Arg description", false)
+            .withArgument("Test22", "Arg description 2", false)
+            .build();
+
+        // when / then
+        checkArgumentCount(command, 2, 2);
+    }
+
+    @Test
+    public void shouldComputeMinAndMaxOnCommandIncludingOptionalArgs() {
+        // given
+        CommandDescription command = getBuilderForArgsTest()
+            .withArgument("arg1", "Arg description", false)
+            .withArgument("arg2", "Arg description 2", true)
+            .withArgument("arg3", "Arg description 3", true)
+            .build();
+
+        // when / then
+        checkArgumentCount(command, 1, 3);
+    }
+
+
+    private static void checkArgumentCount(CommandDescription command, int expectedMin, int expectedMax) {
+        assertThat(CommandUtils.getMinNumberOfArguments(command), equalTo(expectedMin));
+        assertThat(CommandUtils.getMaxNumberOfArguments(command), equalTo(expectedMax));
+    }
+
+    private static CommandDescription.CommandBuilder getBuilderForArgsTest() {
+        return CommandDescription.builder()
+            .labels("authme", "auth")
+            .description("Base")
+            .detailedDescription("Test base command.")
+            .executableCommand(mock(ExecutableCommand.class));
+    }
 }

src/test/java/fr/xephi/authme/command/help/HelpProviderTest.java

@@ -0,0 +1,65 @@
+package fr.xephi.authme.command.help;
+
+import fr.xephi.authme.command.CommandDescription;
+import fr.xephi.authme.command.ExecutableCommand;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.util.Arrays;
+import java.util.List;
+
+import static org.hamcrest.Matchers.contains;
+import static org.junit.Assert.assertThat;
+import static org.mockito.Mockito.mock;
+
+/**
+ * Test for {@link HelpProvider}.
+ */
+public class HelpProviderTest {
+
+    private static CommandDescription parent;
+    private static CommandDescription child;
+
+    @BeforeClass
+    public static void setUpCommands() {
+        parent = CommandDescription.builder()
+            .executableCommand(mock(ExecutableCommand.class))
+            .labels("base", "b")
+            .description("Parent")
+            .detailedDescription("Test base command.")
+            .build();
+        child = CommandDescription.builder()
+            .executableCommand(mock(ExecutableCommand.class))
+            .parent(parent)
+            .labels("child", "c")
+            .description("Child")
+            .detailedDescription("Child test command.")
+            .withArgument("Argument", "The argument", false)
+            .build();
+    }
+
+    @Test
+    public void shouldRetainCorrectLabels() {
+        // given
+        List<String> labels = Arrays.asList("b", "child");
+
+        // when
+        List<String> result = HelpProvider.filterCorrectLabels(child, labels);
+
+        // then
+        assertThat(result, contains("b", "child"));
+    }
+
+    @Test
+    public void shouldReplaceIncorrectLabels() {
+        // given
+        List<String> labels = Arrays.asList("base", "wrong");
+
+        // when
+        List<String> result = HelpProvider.filterCorrectLabels(child, labels);
+
+        // then
+        assertThat(result, contains("base", "child"));
+    }
+
+}