Skip to content

Commit

Permalink
Remove support for repeated parameters in commands
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
Eclipse-Dominator authored and damithc committed Jul 19, 2023
1 parent 4592363 commit a976ec9
Show file tree
Hide file tree
Showing 7 changed files with 140 additions and 50 deletions.
3 changes: 0 additions & 3 deletions docs/UserGuide.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,6 @@ AddressBook Level 3 (AB3) is a **desktop app for managing contacts, optimized fo
* Parameters can be in any order.<br>
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.<br>
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.<br>
e.g. if the command specifies `help 123`, it will be interpreted as `help`.

Expand Down
19 changes: 19 additions & 0 deletions src/main/java/seedu/address/logic/Messages.java
Original file line number Diff line number Diff line change
@@ -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;

/**
Expand All @@ -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<String> 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
18 changes: 18 additions & 0 deletions src/main/java/seedu/address/logic/parser/ArgumentMultimap.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -57,4 +61,18 @@ public List<String> 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));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}
Expand Down
89 changes: 72 additions & 17 deletions src/test/java/seedu/address/logic/parser/AddCommandParserTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,18 @@
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 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;
Expand All @@ -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
Expand Down
57 changes: 27 additions & 30 deletions src/test/java/seedu/address/logic/parser/EditCommandParserTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit a976ec9

Please sign in to comment.