From 4a4144d5ca403977bf4a371b94564eca03a91f1c Mon Sep 17 00:00:00 2001 From: lzq Date: Fri, 14 Jul 2023 22:22:22 +0800 Subject: [PATCH] Remove support for repeated parameters in commands If a single-valued parameter is repeated multiple times in a command, only the last occurrence is taken. This allows the user to correct a mistake in a parameter value by simply typing it again, which may be faster than moving the cursor back to the original parameter values and editing it. This feature serves as a good example of 'going the extra mile to optimize the application for target users' (i.e., users who can type fast). However, this feature can cause certain user errors to go unnoticed e.g., unintended repetition caused by a typo in parameter name Alternatives considered: (a) Retain current behavior but give a friendly warning to the user that a parameter was 'overridden'. (b) Remove this feature i.e., treat repeated single-valued parameters as an error, and reject the command with an error message. Alternative (a) requires significantly more complicated code (see #181), steepening the learning curve for for new programmers. Given that this application is primarily meant for training novice programmers, the impact on the learning curve is not worth the benefit of retaining this feature. Therefore, let's do option (b). --- docs/UserGuide.md | 3 - .../java/seedu/address/logic/Messages.java | 19 ++++ .../logic/parser/AddCommandParser.java | 1 + .../logic/parser/ArgumentMultimap.java | 18 ++++ .../logic/parser/EditCommandParser.java | 3 + .../logic/parser/AddCommandParserTest.java | 89 +++++++++++++++---- .../logic/parser/EditCommandParserTest.java | 57 ++++++------ 7 files changed, 140 insertions(+), 50 deletions(-) diff --git a/docs/UserGuide.md b/docs/UserGuide.md index a1f43dd43..fc4a98a42 100644 --- a/docs/UserGuide.md +++ b/docs/UserGuide.md @@ -57,9 +57,6 @@ AddressBook Level 3 (AB3) is a **desktop app for managing contacts, optimized fo * Parameters can be in any order.
e.g. if the command specifies `n/NAME p/PHONE_NUMBER`, `p/PHONE_NUMBER n/NAME` is also acceptable. -* If a parameter is expected only once in the command but you specified it multiple times, only the last occurrence of the parameter will be taken.
- e.g. if you specify `p/12341234 p/56785678`, only `p/56785678` will be taken. - * Extraneous parameters for commands that do not take in parameters (such as `help`, `list`, `exit` and `clear`) will be ignored.
e.g. if the command specifies `help 123`, it will be interpreted as `help`. diff --git a/src/main/java/seedu/address/logic/Messages.java b/src/main/java/seedu/address/logic/Messages.java index fcf0263b9..ecd32c31b 100644 --- a/src/main/java/seedu/address/logic/Messages.java +++ b/src/main/java/seedu/address/logic/Messages.java @@ -1,5 +1,10 @@ package seedu.address.logic; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import seedu.address.logic.parser.Prefix; import seedu.address.model.person.Person; /** @@ -11,6 +16,20 @@ 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 = + "Multiple values specified for the following single-valued field(s): "; + + /** + * Returns an error message indicating the duplicate prefixes. + */ + public static String getErrorMessageForDuplicatePrefixes(Prefix... duplicatePrefixes) { + assert duplicatePrefixes.length > 0; + + Set duplicateFields = + Stream.of(duplicatePrefixes).map(Prefix::toString).collect(Collectors.toSet()); + + return MESSAGE_DUPLICATE_FIELDS + String.join(" ", duplicateFields); + } /** * Formats the {@code person} for display to the user. diff --git a/src/main/java/seedu/address/logic/parser/AddCommandParser.java b/src/main/java/seedu/address/logic/parser/AddCommandParser.java index 0517d61f0..4ff1a97ed 100644 --- a/src/main/java/seedu/address/logic/parser/AddCommandParser.java +++ b/src/main/java/seedu/address/logic/parser/AddCommandParser.java @@ -38,6 +38,7 @@ public AddCommand parse(String args) throws ParseException { throw new ParseException(String.format(MESSAGE_INVALID_COMMAND_FORMAT, AddCommand.MESSAGE_USAGE)); } + argMultimap.verifyNoDuplicatePrefixesFor(PREFIX_NAME, PREFIX_PHONE, PREFIX_EMAIL, PREFIX_ADDRESS); Name name = ParserUtil.parseName(argMultimap.getValue(PREFIX_NAME).get()); Phone phone = ParserUtil.parsePhone(argMultimap.getValue(PREFIX_PHONE).get()); Email email = ParserUtil.parseEmail(argMultimap.getValue(PREFIX_EMAIL).get()); diff --git a/src/main/java/seedu/address/logic/parser/ArgumentMultimap.java b/src/main/java/seedu/address/logic/parser/ArgumentMultimap.java index 954c8e18f..21e26887a 100644 --- a/src/main/java/seedu/address/logic/parser/ArgumentMultimap.java +++ b/src/main/java/seedu/address/logic/parser/ArgumentMultimap.java @@ -5,6 +5,10 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.stream.Stream; + +import seedu.address.logic.Messages; +import seedu.address.logic.parser.exceptions.ParseException; /** * Stores mapping of prefixes to their respective arguments. @@ -57,4 +61,18 @@ public List getAllValues(Prefix prefix) { public String getPreamble() { return getValue(new Prefix("")).orElse(""); } + + /** + * Throws a {@code ParseException} if any of the prefixes given in {@code prefixes} appeared more than + * once among the arguments. + */ + public void verifyNoDuplicatePrefixesFor(Prefix... prefixes) throws ParseException { + Prefix[] duplicatedPrefixes = Stream.of(prefixes).distinct() + .filter(prefix -> argMultimap.containsKey(prefix) && argMultimap.get(prefix).size() > 1) + .toArray(Prefix[]::new); + + if (duplicatedPrefixes.length > 0) { + throw new ParseException(Messages.getErrorMessageForDuplicatePrefixes(duplicatedPrefixes)); + } + } } diff --git a/src/main/java/seedu/address/logic/parser/EditCommandParser.java b/src/main/java/seedu/address/logic/parser/EditCommandParser.java index a24bec760..46b3309a7 100644 --- a/src/main/java/seedu/address/logic/parser/EditCommandParser.java +++ b/src/main/java/seedu/address/logic/parser/EditCommandParser.java @@ -42,7 +42,10 @@ public EditCommand parse(String args) throws ParseException { throw new ParseException(String.format(MESSAGE_INVALID_COMMAND_FORMAT, EditCommand.MESSAGE_USAGE), pe); } + argMultimap.verifyNoDuplicatePrefixesFor(PREFIX_NAME, PREFIX_PHONE, PREFIX_EMAIL, PREFIX_ADDRESS); + EditPersonDescriptor editPersonDescriptor = new EditPersonDescriptor(); + if (argMultimap.getValue(PREFIX_NAME).isPresent()) { editPersonDescriptor.setName(ParserUtil.parseName(argMultimap.getValue(PREFIX_NAME).get())); } diff --git a/src/test/java/seedu/address/logic/parser/AddCommandParserTest.java b/src/test/java/seedu/address/logic/parser/AddCommandParserTest.java index d0d91a025..5bc11d3cd 100644 --- a/src/test/java/seedu/address/logic/parser/AddCommandParserTest.java +++ b/src/test/java/seedu/address/logic/parser/AddCommandParserTest.java @@ -24,6 +24,10 @@ 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; @@ -31,6 +35,7 @@ import org.junit.jupiter.api.Test; +import seedu.address.logic.Messages; import seedu.address.logic.commands.AddCommand; import seedu.address.model.person.Address; import seedu.address.model.person.Email; @@ -51,27 +56,77 @@ 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 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)); - - // 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 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)); + assertParseSuccess(parser, + NAME_DESC_BOB + PHONE_DESC_BOB + EMAIL_DESC_BOB + ADDRESS_DESC_BOB + TAG_DESC_HUSBAND + TAG_DESC_FRIEND, + new AddCommand(expectedPersonMultipleTags)); + } + + @Test + public void parse_repeatedNonTagValue_failure() { + String validExpectedPersonString = NAME_DESC_BOB + PHONE_DESC_BOB + EMAIL_DESC_BOB + + ADDRESS_DESC_BOB + TAG_DESC_FRIEND; + + // multiple names + assertParseFailure(parser, NAME_DESC_AMY + validExpectedPersonString, + Messages.getErrorMessageForDuplicatePrefixes(PREFIX_NAME)); + + // multiple phones + assertParseFailure(parser, PHONE_DESC_AMY + validExpectedPersonString, + Messages.getErrorMessageForDuplicatePrefixes(PREFIX_PHONE)); + + // multiple emails + assertParseFailure(parser, EMAIL_DESC_AMY + validExpectedPersonString, + Messages.getErrorMessageForDuplicatePrefixes(PREFIX_EMAIL)); + + // multiple addresses + assertParseFailure(parser, ADDRESS_DESC_AMY + validExpectedPersonString, + Messages.getErrorMessageForDuplicatePrefixes(PREFIX_ADDRESS)); + + // multiple fields repeated + assertParseFailure(parser, + validExpectedPersonString + PHONE_DESC_AMY + EMAIL_DESC_AMY + NAME_DESC_AMY + ADDRESS_DESC_AMY + + validExpectedPersonString, + Messages.getErrorMessageForDuplicatePrefixes(PREFIX_NAME, PREFIX_ADDRESS, PREFIX_EMAIL, PREFIX_PHONE)); + + // invalid value followed by valid value + + // invalid name + assertParseFailure(parser, INVALID_NAME_DESC + validExpectedPersonString, + Messages.getErrorMessageForDuplicatePrefixes(PREFIX_NAME)); + + // invalid email + assertParseFailure(parser, INVALID_EMAIL_DESC + validExpectedPersonString, + Messages.getErrorMessageForDuplicatePrefixes(PREFIX_EMAIL)); + + // invalid phone + assertParseFailure(parser, INVALID_PHONE_DESC + validExpectedPersonString, + Messages.getErrorMessageForDuplicatePrefixes(PREFIX_PHONE)); + + // invalid address + assertParseFailure(parser, INVALID_ADDRESS_DESC + validExpectedPersonString, + Messages.getErrorMessageForDuplicatePrefixes(PREFIX_ADDRESS)); + + // valid value followed by invalid value + + // invalid name + assertParseFailure(parser, validExpectedPersonString + INVALID_NAME_DESC, + Messages.getErrorMessageForDuplicatePrefixes(PREFIX_NAME)); + + // invalid email + assertParseFailure(parser, validExpectedPersonString + INVALID_EMAIL_DESC, + Messages.getErrorMessageForDuplicatePrefixes(PREFIX_EMAIL)); + + // invalid phone + assertParseFailure(parser, validExpectedPersonString + INVALID_PHONE_DESC, + Messages.getErrorMessageForDuplicatePrefixes(PREFIX_PHONE)); + + // invalid address + assertParseFailure(parser, validExpectedPersonString + INVALID_ADDRESS_DESC, + Messages.getErrorMessageForDuplicatePrefixes(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 index d252de934..cc7175172 100644 --- a/src/test/java/seedu/address/logic/parser/EditCommandParserTest.java +++ b/src/test/java/seedu/address/logic/parser/EditCommandParserTest.java @@ -16,14 +16,15 @@ 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; @@ -34,6 +35,7 @@ import org.junit.jupiter.api.Test; import seedu.address.commons.core.index.Index; +import seedu.address.logic.Messages; import seedu.address.logic.commands.EditCommand; import seedu.address.logic.commands.EditCommand.EditPersonDescriptor; import seedu.address.model.person.Address; @@ -90,10 +92,6 @@ public void parse_invalidValue_failure() { // invalid phone followed by valid email assertParseFailure(parser, "1" + INVALID_PHONE_DESC + EMAIL_DESC_AMY, Phone.MESSAGE_CONSTRAINTS); - // valid phone followed by invalid phone. The test case for invalid phone followed by valid phone - // is tested at {@code parse_invalidValueFollowedByValidValue_success()} - assertParseFailure(parser, "1" + PHONE_DESC_BOB + INVALID_PHONE_DESC, Phone.MESSAGE_CONSTRAINTS); - // while parsing {@code PREFIX_TAG} alone will reset the tags of the {@code Person} being edited, // parsing it together with a valid tag results in error assertParseFailure(parser, "1" + TAG_DESC_FRIEND + TAG_DESC_HUSBAND + TAG_EMPTY, Tag.MESSAGE_CONSTRAINTS); @@ -166,36 +164,35 @@ public void parse_oneFieldSpecified_success() { } @Test - public void parse_multipleRepeatedFields_acceptsLast() { + public void parse_multipleRepeatedFields_failure() { + // More extensive testing of duplicate parameter detections is done in + // AddCommandParserTest#parse_repeatedNonTagValue_failure() + + // valid followed by invalid Index targetIndex = INDEX_FIRST_PERSON; - String userInput = targetIndex.getOneBased() + PHONE_DESC_AMY + ADDRESS_DESC_AMY + EMAIL_DESC_AMY + String userInput = targetIndex.getOneBased() + INVALID_PHONE_DESC + PHONE_DESC_BOB; + + assertParseFailure(parser, userInput, Messages.getErrorMessageForDuplicatePrefixes(PREFIX_PHONE)); + + // invalid followed by valid + userInput = targetIndex.getOneBased() + PHONE_DESC_BOB + INVALID_PHONE_DESC; + + assertParseFailure(parser, userInput, Messages.getErrorMessageForDuplicatePrefixes(PREFIX_PHONE)); + + // mulltiple valid fields repeated + 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); + assertParseFailure(parser, userInput, + Messages.getErrorMessageForDuplicatePrefixes(PREFIX_PHONE, PREFIX_EMAIL, PREFIX_ADDRESS)); - assertParseSuccess(parser, userInput, expectedCommand); - } + // multiple invalid values + userInput = targetIndex.getOneBased() + INVALID_PHONE_DESC + INVALID_ADDRESS_DESC + INVALID_EMAIL_DESC + + INVALID_PHONE_DESC + INVALID_ADDRESS_DESC + INVALID_EMAIL_DESC; - @Test - public void parse_invalidValueFollowedByValidValue_success() { - // 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); - - // 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.getErrorMessageForDuplicatePrefixes(PREFIX_PHONE, PREFIX_EMAIL, PREFIX_ADDRESS)); } @Test