Removal of repeatable parameters

The behavior of repeated parameters in AddCommand and EditCommand is to take the last value used. This can be confusing and hard to work with. Therefore, instead of allowing repeatable parameters, Let's change it to display a Command Error to the users instead.
se-edu · Jun 6, 2023 · ba48384 · ba48384
1 parent 2215c93
commit ba48384
Show file tree

Hide file tree

Showing 6 changed files with 129 additions and 34 deletions.
diff --git a/src/main/java/seedu/address/commons/core/Messages.java b/src/main/java/seedu/address/commons/core/Messages.java
@@ -1,5 +1,10 @@
 package seedu.address.commons.core;
 
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import seedu.address.logic.parser.Prefix;
+
 /**
  * Container for user visible messages.
  */
@@ -9,5 +14,22 @@ public class Messages {
     public static final String MESSAGE_INVALID_COMMAND_FORMAT = "Invalid command format! \n%1$s";
     public static final String MESSAGE_INVALID_PERSON_DISPLAYED_INDEX = "The person index provided is invalid";
     public static final String MESSAGE_PERSONS_LISTED_OVERVIEW = "%1$d persons listed!";
+    public static final String MESSAGE_DUPLICATE_FIELDS = "Detected multiple values for the following field%s: "
+            + "%s.\nPlease only a single value for those fields\n";
+
+    /**
+     * Formats the duplicate prefixes into an error message.
+     */
+    public static String formatDuplicatePrefixesToMessage(Set<Prefix> duplicatePrefixes) {
+        if (duplicatePrefixes.size() == 0) {
+            return "";
+        }
+
+        Set<String> duplicateFields = duplicatePrefixes.stream().map(Prefix::toString).collect(Collectors.toSet());
+
+
+        return String.format(MESSAGE_DUPLICATE_FIELDS, duplicateFields.size() > 1 ? "s" : "",
+                String.join(" ", duplicateFields));
+    }
 
 }
diff --git a/src/main/java/seedu/address/logic/parser/AddCommandParser.java b/src/main/java/seedu/address/logic/parser/AddCommandParser.java
@@ -10,6 +10,7 @@
 import java.util.Set;
 import java.util.stream.Stream;
 
+import seedu.address.commons.core.Messages;
 import seedu.address.logic.commands.AddCommand;
 import seedu.address.logic.parser.exceptions.ParseException;
 import seedu.address.model.person.Address;
@@ -44,6 +45,13 @@ public AddCommand parse(String args) throws ParseException {
         Address address = ParserUtil.parseAddress(argMultimap.getValue(PREFIX_ADDRESS).get());
         Set<Tag> tagList = ParserUtil.parseTags(argMultimap.getAllValues(PREFIX_TAG));
 
+        Set<Prefix> duplicatePrefixes = argMultimap.getDuplicatePrefixes(PREFIX_NAME, PREFIX_PHONE,
+                PREFIX_EMAIL, PREFIX_ADDRESS);
+
+        if (!duplicatePrefixes.isEmpty()) {
+            throw new ParseException(Messages.formatDuplicatePrefixesToMessage(duplicatePrefixes));
+        }
+
         Person person = new Person(name, phone, email, address, tagList);
 
         return new AddCommand(person);

diff --git a/src/main/java/seedu/address/logic/parser/ArgumentMultimap.java b/src/main/java/seedu/address/logic/parser/ArgumentMultimap.java
@@ -2,9 +2,11 @@
 
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 
 /**
  * Stores mapping of prefixes to their respective arguments.
@@ -57,4 +59,19 @@ public List<String> getAllValues(Prefix prefix) {
     public String getPreamble() {
         return getValue(new Prefix("")).orElse("");
     }
+
+    /**
+     * Returns a set of all prefixes given in the {@code prefixes} that appeared more than once in
+     * the argument multimap.
+     */
+    public Set<Prefix> getDuplicatePrefixes(Prefix... prefixes) {
+        Set<Prefix> ret = new HashSet<>();
+        Set<Prefix> prefixSet = Set.of(prefixes);
+        argMultimap.forEach((key, list) -> {
+            if (list.size() > 1 && prefixSet.contains(key)) {
+                ret.add(key);
+            }
+        });
+        return ret;
+    }
 }
diff --git a/src/main/java/seedu/address/logic/parser/EditCommandParser.java b/src/main/java/seedu/address/logic/parser/EditCommandParser.java
@@ -13,6 +13,7 @@
 import java.util.Optional;
 import java.util.Set;
 
+import seedu.address.commons.core.Messages;
 import seedu.address.commons.core.index.Index;
 import seedu.address.logic.commands.EditCommand;
 import seedu.address.logic.commands.EditCommand.EditPersonDescriptor;
@@ -43,6 +44,7 @@ public EditCommand parse(String args) throws ParseException {
         }
 
         EditPersonDescriptor editPersonDescriptor = new EditPersonDescriptor();
+
         if (argMultimap.getValue(PREFIX_NAME).isPresent()) {
             editPersonDescriptor.setName(ParserUtil.parseName(argMultimap.getValue(PREFIX_NAME).get()));
         }
@@ -57,6 +59,13 @@ public EditCommand parse(String args) throws ParseException {
         }
         parseTagsForEdit(argMultimap.getAllValues(PREFIX_TAG)).ifPresent(editPersonDescriptor::setTags);
 
+        Set<Prefix> duplicatePrefixes = argMultimap.getDuplicatePrefixes(PREFIX_NAME, PREFIX_PHONE,
+                PREFIX_EMAIL, PREFIX_ADDRESS);
+
+        if (!duplicatePrefixes.isEmpty()) {
+            throw new ParseException(Messages.formatDuplicatePrefixesToMessage(duplicatePrefixes));
+        }
+
         if (!editPersonDescriptor.isAnyFieldEdited()) {
             throw new ParseException(EditCommand.MESSAGE_NOT_EDITED);
         }
@@ -78,5 +87,4 @@ private Optional<Set<Tag>> parseTagsForEdit(Collection<String> tags) throws Pars
         Collection<String> tagSet = tags.size() == 1 && tags.contains("") ? Collections.emptySet() : tags;
         return Optional.of(ParserUtil.parseTags(tagSet));
     }
-
 }
diff --git a/src/test/java/seedu/address/logic/parser/AddCommandParserTest.java b/src/test/java/seedu/address/logic/parser/AddCommandParserTest.java
@@ -24,13 +24,20 @@
 import static seedu.address.logic.commands.CommandTestUtil.VALID_PHONE_BOB;
 import static seedu.address.logic.commands.CommandTestUtil.VALID_TAG_FRIEND;
 import static seedu.address.logic.commands.CommandTestUtil.VALID_TAG_HUSBAND;
+import static seedu.address.logic.parser.CliSyntax.PREFIX_ADDRESS;
+import static seedu.address.logic.parser.CliSyntax.PREFIX_EMAIL;
+import static seedu.address.logic.parser.CliSyntax.PREFIX_NAME;
+import static seedu.address.logic.parser.CliSyntax.PREFIX_PHONE;
 import static seedu.address.logic.parser.CommandParserTestUtil.assertParseFailure;
 import static seedu.address.logic.parser.CommandParserTestUtil.assertParseSuccess;
 import static seedu.address.testutil.TypicalPersons.AMY;
 import static seedu.address.testutil.TypicalPersons.BOB;
 
+import java.util.Set;
+
 import org.junit.jupiter.api.Test;
 
+import seedu.address.commons.core.Messages;
 import seedu.address.logic.commands.AddCommand;
 import seedu.address.model.person.Address;
 import seedu.address.model.person.Email;
@@ -51,27 +58,59 @@ public void parse_allFieldsPresent_success() {
         assertParseSuccess(parser, PREAMBLE_WHITESPACE + NAME_DESC_BOB + PHONE_DESC_BOB + EMAIL_DESC_BOB
                 + ADDRESS_DESC_BOB + TAG_DESC_FRIEND, new AddCommand(expectedPerson));
 
-        // multiple names - last name accepted
-        assertParseSuccess(parser, NAME_DESC_AMY + NAME_DESC_BOB + PHONE_DESC_BOB + EMAIL_DESC_BOB
-                + ADDRESS_DESC_BOB + TAG_DESC_FRIEND, new AddCommand(expectedPerson));
 
-        // multiple phones - last phone accepted
-        assertParseSuccess(parser, NAME_DESC_BOB + PHONE_DESC_AMY + PHONE_DESC_BOB + EMAIL_DESC_BOB
-                + ADDRESS_DESC_BOB + TAG_DESC_FRIEND, new AddCommand(expectedPerson));
+        // multiple tags - all accepted
+        Person expectedPersonMultipleTags = new PersonBuilder(BOB)
+                .withTags(VALID_TAG_FRIEND, VALID_TAG_HUSBAND).build();
+        assertParseSuccess(parser, NAME_DESC_BOB + PHONE_DESC_BOB + EMAIL_DESC_BOB + ADDRESS_DESC_BOB
+                 + TAG_DESC_HUSBAND + TAG_DESC_FRIEND, new AddCommand(expectedPersonMultipleTags));
+    }
 
-        // multiple emails - last email accepted
-        assertParseSuccess(parser, NAME_DESC_BOB + PHONE_DESC_BOB + EMAIL_DESC_AMY + EMAIL_DESC_BOB
-                + ADDRESS_DESC_BOB + TAG_DESC_FRIEND, new AddCommand(expectedPerson));
+    @Test
+    public void parse_repeatedNonTagValue_success() {
+        String validExpectedPerson = NAME_DESC_BOB + PHONE_DESC_BOB + EMAIL_DESC_BOB
+                + ADDRESS_DESC_BOB + TAG_DESC_FRIEND;
 
-        // multiple addresses - last address accepted
-        assertParseSuccess(parser, NAME_DESC_BOB + PHONE_DESC_BOB + EMAIL_DESC_BOB + ADDRESS_DESC_AMY
-                + ADDRESS_DESC_BOB + TAG_DESC_FRIEND, new AddCommand(expectedPerson));
+        // multiple fields repeated
+        assertParseFailure(parser,
+                validExpectedPerson + PHONE_DESC_AMY + EMAIL_DESC_AMY + NAME_DESC_AMY
+                        + ADDRESS_DESC_AMY + validExpectedPerson,
+                Messages.formatDuplicatePrefixesToMessage(Set.of(PREFIX_NAME,
+                        PREFIX_ADDRESS, PREFIX_EMAIL, PREFIX_PHONE)));
 
-        // multiple tags - all accepted
-        Person expectedPersonMultipleTags = new PersonBuilder(BOB).withTags(VALID_TAG_FRIEND, VALID_TAG_HUSBAND)
-                .build();
-        assertParseSuccess(parser, NAME_DESC_BOB + PHONE_DESC_BOB + EMAIL_DESC_BOB + ADDRESS_DESC_BOB
-                + TAG_DESC_HUSBAND + TAG_DESC_FRIEND, new AddCommand(expectedPersonMultipleTags));
+        // multiple names
+        assertParseFailure(parser, NAME_DESC_AMY + validExpectedPerson,
+                Messages.formatDuplicatePrefixesToMessage(Set.of(PREFIX_NAME)));
+
+        // multiple phones
+        assertParseFailure(parser, PHONE_DESC_AMY + validExpectedPerson,
+                Messages.formatDuplicatePrefixesToMessage(Set.of(PREFIX_PHONE)));
+
+        // multiple emails
+        assertParseFailure(parser, EMAIL_DESC_AMY + validExpectedPerson,
+                Messages.formatDuplicatePrefixesToMessage(Set.of(PREFIX_EMAIL)));
+
+        // multiple addresses
+        assertParseFailure(parser, ADDRESS_DESC_AMY + validExpectedPerson,
+                Messages.formatDuplicatePrefixesToMessage(Set.of(PREFIX_ADDRESS)));
+
+        // invalid value followed by valid value
+
+        // invalid name
+        assertParseFailure(parser, INVALID_NAME_DESC + validExpectedPerson,
+                Messages.formatDuplicatePrefixesToMessage(Set.of(PREFIX_NAME)));
+
+        // invalid email
+        assertParseFailure(parser, INVALID_EMAIL_DESC + validExpectedPerson,
+                Messages.formatDuplicatePrefixesToMessage(Set.of(PREFIX_EMAIL)));
+
+        // invalid phone
+        assertParseFailure(parser, INVALID_PHONE_DESC + validExpectedPerson,
+                Messages.formatDuplicatePrefixesToMessage(Set.of(PREFIX_PHONE)));
+
+        // invalid address
+        assertParseFailure(parser, INVALID_ADDRESS_DESC + validExpectedPerson,
+                Messages.formatDuplicatePrefixesToMessage(Set.of(PREFIX_ADDRESS)));
     }
 
     @Test

diff --git a/src/test/java/seedu/address/logic/parser/EditCommandParserTest.java b/src/test/java/seedu/address/logic/parser/EditCommandParserTest.java
@@ -16,23 +16,27 @@
 import static seedu.address.logic.commands.CommandTestUtil.TAG_DESC_FRIEND;
 import static seedu.address.logic.commands.CommandTestUtil.TAG_DESC_HUSBAND;
 import static seedu.address.logic.commands.CommandTestUtil.VALID_ADDRESS_AMY;
-import static seedu.address.logic.commands.CommandTestUtil.VALID_ADDRESS_BOB;
 import static seedu.address.logic.commands.CommandTestUtil.VALID_EMAIL_AMY;
-import static seedu.address.logic.commands.CommandTestUtil.VALID_EMAIL_BOB;
 import static seedu.address.logic.commands.CommandTestUtil.VALID_NAME_AMY;
 import static seedu.address.logic.commands.CommandTestUtil.VALID_PHONE_AMY;
 import static seedu.address.logic.commands.CommandTestUtil.VALID_PHONE_BOB;
 import static seedu.address.logic.commands.CommandTestUtil.VALID_TAG_FRIEND;
 import static seedu.address.logic.commands.CommandTestUtil.VALID_TAG_HUSBAND;
+import static seedu.address.logic.parser.CliSyntax.PREFIX_ADDRESS;
+import static seedu.address.logic.parser.CliSyntax.PREFIX_EMAIL;
+import static seedu.address.logic.parser.CliSyntax.PREFIX_PHONE;
 import static seedu.address.logic.parser.CliSyntax.PREFIX_TAG;
 import static seedu.address.logic.parser.CommandParserTestUtil.assertParseFailure;
 import static seedu.address.logic.parser.CommandParserTestUtil.assertParseSuccess;
 import static seedu.address.testutil.TypicalIndexes.INDEX_FIRST_PERSON;
 import static seedu.address.testutil.TypicalIndexes.INDEX_SECOND_PERSON;
 import static seedu.address.testutil.TypicalIndexes.INDEX_THIRD_PERSON;
 
+import java.util.Set;
+
 import org.junit.jupiter.api.Test;
 
+import seedu.address.commons.core.Messages;
 import seedu.address.commons.core.index.Index;
 import seedu.address.logic.commands.EditCommand;
 import seedu.address.logic.commands.EditCommand.EditPersonDescriptor;
@@ -166,36 +170,33 @@ public void parse_oneFieldSpecified_success() {
     }
 
     @Test
-    public void parse_multipleRepeatedFields_acceptsLast() {
+    public void parse_multipleRepeatedFields_failure() {
         Index targetIndex = INDEX_FIRST_PERSON;
         String userInput = targetIndex.getOneBased() + PHONE_DESC_AMY + ADDRESS_DESC_AMY + EMAIL_DESC_AMY
                 + TAG_DESC_FRIEND + PHONE_DESC_AMY + ADDRESS_DESC_AMY + EMAIL_DESC_AMY + TAG_DESC_FRIEND
                 + PHONE_DESC_BOB + ADDRESS_DESC_BOB + EMAIL_DESC_BOB + TAG_DESC_HUSBAND;
 
-        EditPersonDescriptor descriptor = new EditPersonDescriptorBuilder().withPhone(VALID_PHONE_BOB)
-                .withEmail(VALID_EMAIL_BOB).withAddress(VALID_ADDRESS_BOB).withTags(VALID_TAG_FRIEND, VALID_TAG_HUSBAND)
-                .build();
-        EditCommand expectedCommand = new EditCommand(targetIndex, descriptor);
+        Set<Prefix> duplicatePrefixes = Set.of(PREFIX_PHONE, PREFIX_EMAIL, PREFIX_ADDRESS);
 
-        assertParseSuccess(parser, userInput, expectedCommand);
+        assertParseFailure(parser, userInput,
+                Messages.formatDuplicatePrefixesToMessage(duplicatePrefixes));
     }
 
     @Test
-    public void parse_invalidValueFollowedByValidValue_success() {
+    public void parse_invalidValueFollowedByValidValue_failure() {
         // no other valid values specified
         Index targetIndex = INDEX_FIRST_PERSON;
         String userInput = targetIndex.getOneBased() + INVALID_PHONE_DESC + PHONE_DESC_BOB;
-        EditPersonDescriptor descriptor = new EditPersonDescriptorBuilder().withPhone(VALID_PHONE_BOB).build();
-        EditCommand expectedCommand = new EditCommand(targetIndex, descriptor);
-        assertParseSuccess(parser, userInput, expectedCommand);
+        Set<Prefix> duplicatePrefixes = Set.of(PREFIX_PHONE);
+
+        assertParseFailure(parser, userInput,
+                Messages.formatDuplicatePrefixesToMessage(duplicatePrefixes));
 
         // other valid values specified
         userInput = targetIndex.getOneBased() + EMAIL_DESC_BOB + INVALID_PHONE_DESC + ADDRESS_DESC_BOB
                 + PHONE_DESC_BOB;
-        descriptor = new EditPersonDescriptorBuilder().withPhone(VALID_PHONE_BOB).withEmail(VALID_EMAIL_BOB)
-                .withAddress(VALID_ADDRESS_BOB).build();
-        expectedCommand = new EditCommand(targetIndex, descriptor);
-        assertParseSuccess(parser, userInput, expectedCommand);
+        assertParseFailure(parser, userInput,
+                Messages.formatDuplicatePrefixesToMessage(duplicatePrefixes));
     }
 
     @Test