Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removal of repeatable parameters #190

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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>
Eclipse-Dominator marked this conversation as resolved.
Show resolved Hide resolved
e.g. if the command specifies `n/NAME p/PHONE_NUMBER`, `p/PHONE_NUMBER n/NAME` is also acceptable.
Eclipse-Dominator marked this conversation as resolved.
Show resolved Hide resolved

* 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`.
Eclipse-Dominator marked this conversation as resolved.
Show resolved Hide resolved

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";
Eclipse-Dominator marked this conversation as resolved.
Show resolved Hide resolved
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
Eclipse-Dominator marked this conversation as resolved.
Show resolved Hide resolved
* once among the arguments.
*/
public void verifyNoDuplicatePrefixesFor(Prefix... prefixes) throws ParseException {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could consider shortening the name of the method to verifyNoDuplicatePrefixes as the 'For' does not seem to be very necessary

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It reads as a natural statement e.g., verifyNoDuplicatePrefixesFor(PREFIX_NAME, PREFIX_ADDRESS) i.e., verify no duplicate prefixes for A, B, ..
But OK to remove.
Can also consider verifyNoDuplicatesFor as Prefix is implied by the parameter type.

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);

Eclipse-Dominator marked this conversation as resolved.
Show resolved Hide resolved
// 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() {
Eclipse-Dominator marked this conversation as resolved.
Show resolved Hide resolved
// 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
Loading