Skip to content

Commit

Permalink
Removal of repeatable parameters as feature
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Eclipse-Dominator committed Jun 12, 2023
1 parent f0e9069 commit 7d3bb2b
Show file tree
Hide file tree
Showing 6 changed files with 124 additions and 33 deletions.
21 changes: 21 additions & 0 deletions src/main/java/seedu/address/commons/core/Messages.java
Original file line number Diff line number Diff line change
@@ -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.
*/
Expand All @@ -9,5 +14,21 @@ 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 getDuplicatePrefixesToMessage(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));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ 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));

argMultimap.verifyNoDuplicatePrefixesFor(PREFIX_NAME, PREFIX_PHONE, PREFIX_EMAIL, PREFIX_ADDRESS);

Person person = new Person(name, phone, email, address, tagList);

return new AddCommand(person);
Expand Down
25 changes: 25 additions & 0 deletions src/main/java/seedu/address/logic/parser/ArgumentMultimap.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,14 @@

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;

import seedu.address.commons.core.Messages;
import seedu.address.logic.parser.exceptions.ParseException;

/**
* Stores mapping of prefixes to their respective arguments.
Expand Down Expand Up @@ -57,4 +62,24 @@ public List<String> getAllValues(Prefix prefix) {
public String getPreamble() {
return getValue(new Prefix("")).orElse("");
}

/**
* throws a ParseException if any of the prefixes given in {@code prefixes} appeared more than
* once in the argument multimap.
*/
public void verifyNoDuplicatePrefixesFor(Prefix... prefixes) throws ParseException {
Set<Prefix> duplicatedPrefixes = new HashSet<>();
Set<Prefix> prefixSet = Set.of(prefixes);
argMultimap.forEach((key, list) -> {
if (list.size() > 1 && prefixSet.contains(key)) {
duplicatedPrefixes.add(key);
}
});

if (duplicatedPrefixes.isEmpty()) {
return;
}

throw new ParseException(Messages.getDuplicatePrefixesToMessage(duplicatedPrefixes));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,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()));
}
Expand All @@ -57,6 +58,8 @@ public EditCommand parse(String args) throws ParseException {
}
parseTagsForEdit(argMultimap.getAllValues(PREFIX_TAG)).ifPresent(editPersonDescriptor::setTags);

argMultimap.verifyNoDuplicatePrefixesFor(PREFIX_NAME, PREFIX_PHONE, PREFIX_EMAIL, PREFIX_ADDRESS);

if (!editPersonDescriptor.isAnyFieldEdited()) {
throw new ParseException(EditCommand.MESSAGE_NOT_EDITED);
}
Expand Down
73 changes: 56 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,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;
Expand All @@ -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_exceptionThrown() {
String validExpectedPersonString = 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 names
assertParseFailure(parser, NAME_DESC_AMY + validExpectedPersonString,
Messages.getDuplicatePrefixesToMessage(Set.of(PREFIX_NAME)));

// 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 phones
assertParseFailure(parser, PHONE_DESC_AMY + validExpectedPersonString,
Messages.getDuplicatePrefixesToMessage(Set.of(PREFIX_PHONE)));

// multiple emails
assertParseFailure(parser, EMAIL_DESC_AMY + validExpectedPersonString,
Messages.getDuplicatePrefixesToMessage(Set.of(PREFIX_EMAIL)));

// multiple addresses
assertParseFailure(parser, ADDRESS_DESC_AMY + validExpectedPersonString,
Messages.getDuplicatePrefixesToMessage(Set.of(PREFIX_ADDRESS)));

// invalid value followed by valid value

// invalid name
assertParseFailure(parser, INVALID_NAME_DESC + validExpectedPersonString,
Messages.getDuplicatePrefixesToMessage(Set.of(PREFIX_NAME)));

// invalid email
assertParseFailure(parser, INVALID_EMAIL_DESC + validExpectedPersonString,
Messages.getDuplicatePrefixesToMessage(Set.of(PREFIX_EMAIL)));

// invalid phone
assertParseFailure(parser, INVALID_PHONE_DESC + validExpectedPersonString,
Messages.getDuplicatePrefixesToMessage(Set.of(PREFIX_PHONE)));

// invalid address
assertParseFailure(parser, INVALID_ADDRESS_DESC + validExpectedPersonString,
Messages.getDuplicatePrefixesToMessage(Set.of(PREFIX_ADDRESS)));

// multiple fields repeated
assertParseFailure(parser,
validExpectedPersonString + PHONE_DESC_AMY + EMAIL_DESC_AMY + NAME_DESC_AMY
+ ADDRESS_DESC_AMY + validExpectedPersonString,
Messages.getDuplicatePrefixesToMessage(Set.of(PREFIX_NAME,
PREFIX_ADDRESS, PREFIX_EMAIL, PREFIX_PHONE)));
}

@Test
Expand Down
33 changes: 17 additions & 16 deletions src/test/java/seedu/address/logic/parser/EditCommandParserTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.getDuplicatePrefixesToMessage(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.getDuplicatePrefixesToMessage(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.getDuplicatePrefixesToMessage(duplicatePrefixes));
}

@Test
Expand Down

0 comments on commit 7d3bb2b

Please sign in to comment.