From c8296b41fca3b3e8a4aec788edae6bd49de6dbdc Mon Sep 17 00:00:00 2001 From: royleochan Date: Wed, 7 Oct 2020 00:00:08 +0800 Subject: [PATCH] Refactor delete and make contact names unique --- .../seedu/address/commons/core/Messages.java | 1 + .../address/logic/commands/DeleteCommand.java | 29 ++++++++++-------- .../logic/parser/DeleteCommandParser.java | 6 ++-- .../java/seedu/address/model/AddressBook.java | 10 +++++++ src/main/java/seedu/address/model/Model.java | 6 ++++ .../seedu/address/model/ModelManager.java | 7 +++++ .../seedu/address/model/person/Person.java | 13 +++++--- .../model/person/UniquePersonList.java | 8 +++++ .../logic/commands/AddCommandTest.java | 6 ++++ .../logic/commands/DeleteCommandTest.java | 30 ++++++++++--------- .../logic/parser/DeleteCommandParserTest.java | 6 ++-- .../address/model/person/PersonTest.java | 4 +-- .../model/person/UniquePersonListTest.java | 7 ++++- .../seedu/address/testutil/PersonBuilder.java | 5 ++-- 14 files changed, 95 insertions(+), 43 deletions(-) diff --git a/src/main/java/seedu/address/commons/core/Messages.java b/src/main/java/seedu/address/commons/core/Messages.java index 1deb3a1e469..bd1705a86fb 100644 --- a/src/main/java/seedu/address/commons/core/Messages.java +++ b/src/main/java/seedu/address/commons/core/Messages.java @@ -7,6 +7,7 @@ public class Messages { public static final String MESSAGE_UNKNOWN_COMMAND = "Unknown command"; public static final String MESSAGE_INVALID_COMMAND_FORMAT = "Invalid command format! \n%1$s"; + public static final String MESSAGE_INVALID_PERSON_DISPLAYED = "The person provided is not in your contacts"; 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!"; diff --git a/src/main/java/seedu/address/logic/commands/DeleteCommand.java b/src/main/java/seedu/address/logic/commands/DeleteCommand.java index 02fd256acba..0511319b6b4 100644 --- a/src/main/java/seedu/address/logic/commands/DeleteCommand.java +++ b/src/main/java/seedu/address/logic/commands/DeleteCommand.java @@ -3,11 +3,12 @@ import static java.util.Objects.requireNonNull; import java.util.List; +import java.util.stream.Collectors; import seedu.address.commons.core.Messages; -import seedu.address.commons.core.index.Index; import seedu.address.logic.commands.exceptions.CommandException; import seedu.address.model.Model; +import seedu.address.model.person.Name; import seedu.address.model.person.Person; /** @@ -15,31 +16,33 @@ */ public class DeleteCommand extends Command { - public static final String COMMAND_WORD = "delete"; + public static final String COMMAND_WORD = "contact delete"; public static final String MESSAGE_USAGE = COMMAND_WORD - + ": Deletes the person identified by the index number used in the displayed person list.\n" - + "Parameters: INDEX (must be a positive integer)\n" - + "Example: " + COMMAND_WORD + " 1"; + + ": Deletes the person identified by the name used in the displayed person list.\n" + + "Parameters: NAME (must be a valid name)\n" + + "Example: " + COMMAND_WORD + " Roy"; public static final String MESSAGE_DELETE_PERSON_SUCCESS = "Deleted Person: %1$s"; - private final Index targetIndex; + private final Name targetName; - public DeleteCommand(Index targetIndex) { - this.targetIndex = targetIndex; + public DeleteCommand(Name targetName) { + this.targetName = targetName; } @Override public CommandResult execute(Model model) throws CommandException { requireNonNull(model); - List lastShownList = model.getFilteredPersonList(); + boolean isValidContact = model.hasPersonName(targetName); - if (targetIndex.getZeroBased() >= lastShownList.size()) { - throw new CommandException(Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX); + if (!isValidContact) { + throw new CommandException(Messages.MESSAGE_INVALID_PERSON_DISPLAYED); } - Person personToDelete = lastShownList.get(targetIndex.getZeroBased()); + List filteredList = model.getFilteredPersonList().stream() + .filter(person -> person.isSameName(targetName)).collect(Collectors.toList()); + Person personToDelete = filteredList.get(0); model.deletePerson(personToDelete); return new CommandResult(String.format(MESSAGE_DELETE_PERSON_SUCCESS, personToDelete)); } @@ -48,6 +51,6 @@ public CommandResult execute(Model model) throws CommandException { public boolean equals(Object other) { return other == this // short circuit if same object || (other instanceof DeleteCommand // instanceof handles nulls - && targetIndex.equals(((DeleteCommand) other).targetIndex)); // state check + && targetName.equals(((DeleteCommand) other).targetName)); // state check } } diff --git a/src/main/java/seedu/address/logic/parser/DeleteCommandParser.java b/src/main/java/seedu/address/logic/parser/DeleteCommandParser.java index 522b93081cc..22cba911ef8 100644 --- a/src/main/java/seedu/address/logic/parser/DeleteCommandParser.java +++ b/src/main/java/seedu/address/logic/parser/DeleteCommandParser.java @@ -2,9 +2,9 @@ import static seedu.address.commons.core.Messages.MESSAGE_INVALID_COMMAND_FORMAT; -import seedu.address.commons.core.index.Index; import seedu.address.logic.commands.DeleteCommand; import seedu.address.logic.parser.exceptions.ParseException; +import seedu.address.model.person.Name; /** * Parses input arguments and creates a new DeleteCommand object @@ -18,8 +18,8 @@ public class DeleteCommandParser implements Parser { */ public DeleteCommand parse(String args) throws ParseException { try { - Index index = ParserUtil.parseIndex(args); - return new DeleteCommand(index); + Name targetName = ParserUtil.parseName(args); + return new DeleteCommand(targetName); } catch (ParseException pe) { throw new ParseException( String.format(MESSAGE_INVALID_COMMAND_FORMAT, DeleteCommand.MESSAGE_USAGE), pe); diff --git a/src/main/java/seedu/address/model/AddressBook.java b/src/main/java/seedu/address/model/AddressBook.java index 1a943a0781a..5a6402b0bce 100644 --- a/src/main/java/seedu/address/model/AddressBook.java +++ b/src/main/java/seedu/address/model/AddressBook.java @@ -5,9 +5,11 @@ import java.util.List; import javafx.collections.ObservableList; +import seedu.address.model.person.Name; import seedu.address.model.person.Person; import seedu.address.model.person.UniquePersonList; + /** * Wraps all data at the address-book level * Duplicates are not allowed (by .isSamePerson comparison) @@ -66,6 +68,14 @@ public boolean hasPerson(Person person) { return persons.contains(person); } + /** + * Returns true if a person with the same name as {@code person} exists in the address book. + */ + public boolean hasPersonName(Name name) { + requireNonNull(name); + return persons.contains(name); + } + /** * Adds a person to the address book. * The person must not already exist in the address book. diff --git a/src/main/java/seedu/address/model/Model.java b/src/main/java/seedu/address/model/Model.java index d54df471c1f..641d7d3110f 100644 --- a/src/main/java/seedu/address/model/Model.java +++ b/src/main/java/seedu/address/model/Model.java @@ -5,6 +5,7 @@ import javafx.collections.ObservableList; import seedu.address.commons.core.GuiSettings; +import seedu.address.model.person.Name; import seedu.address.model.person.Person; /** @@ -57,6 +58,11 @@ public interface Model { */ boolean hasPerson(Person person); + /** + * Returns true if a person with the same name as {@code person} exists in the address book. + */ + boolean hasPersonName(Name name); + /** * Deletes the given person. * The person must exist in the address book. diff --git a/src/main/java/seedu/address/model/ModelManager.java b/src/main/java/seedu/address/model/ModelManager.java index 0650c954f5c..4e6036c7a49 100644 --- a/src/main/java/seedu/address/model/ModelManager.java +++ b/src/main/java/seedu/address/model/ModelManager.java @@ -11,6 +11,7 @@ import javafx.collections.transformation.FilteredList; import seedu.address.commons.core.GuiSettings; import seedu.address.commons.core.LogsCenter; +import seedu.address.model.person.Name; import seedu.address.model.person.Person; /** @@ -94,6 +95,12 @@ public boolean hasPerson(Person person) { return addressBook.hasPerson(person); } + @Override + public boolean hasPersonName(Name name) { + requireNonNull(name); + return addressBook.hasPersonName(name); + } + @Override public void deletePerson(Person target) { addressBook.removePerson(target); diff --git a/src/main/java/seedu/address/model/person/Person.java b/src/main/java/seedu/address/model/person/Person.java index f43cae3f1a1..2b9e83c3139 100644 --- a/src/main/java/seedu/address/model/person/Person.java +++ b/src/main/java/seedu/address/model/person/Person.java @@ -55,8 +55,7 @@ public Set getTags() { } /** - * Returns true if both persons of the same name have at least one other identity field that is the same. - * This defines a weaker notion of equality between two persons. + * Returns true if both persons have the same name. */ public boolean isSamePerson(Person otherPerson) { if (otherPerson == this) { @@ -64,8 +63,14 @@ public boolean isSamePerson(Person otherPerson) { } return otherPerson != null - && otherPerson.getName().equals(getName()) - && (otherPerson.getPhone().equals(getPhone()) || otherPerson.getEmail().equals(getEmail())); + && otherPerson.getName().equals(getName()); + } + + /** + * Returns true if both persons have the same name. + */ + public boolean isSameName(Name otherName) { + return name.equals(otherName); } /** diff --git a/src/main/java/seedu/address/model/person/UniquePersonList.java b/src/main/java/seedu/address/model/person/UniquePersonList.java index 0fee4fe57e6..4fbce57f4de 100644 --- a/src/main/java/seedu/address/model/person/UniquePersonList.java +++ b/src/main/java/seedu/address/model/person/UniquePersonList.java @@ -36,6 +36,14 @@ public boolean contains(Person toCheck) { return internalList.stream().anyMatch(toCheck::isSamePerson); } + /** + * Returns true if the list contains a person with the same name. + */ + public boolean contains(Name toCheck) { + requireNonNull(toCheck); + return internalList.stream().anyMatch(person -> person.isSameName(toCheck)); + } + /** * Adds a person to the list. * The person must not already exist in the list. diff --git a/src/test/java/seedu/address/logic/commands/AddCommandTest.java b/src/test/java/seedu/address/logic/commands/AddCommandTest.java index 5865713d5dd..a267160b8ef 100644 --- a/src/test/java/seedu/address/logic/commands/AddCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/AddCommandTest.java @@ -20,6 +20,7 @@ import seedu.address.model.Model; import seedu.address.model.ReadOnlyAddressBook; import seedu.address.model.ReadOnlyUserPrefs; +import seedu.address.model.person.Name; import seedu.address.model.person.Person; import seedu.address.testutil.PersonBuilder; @@ -128,6 +129,11 @@ public boolean hasPerson(Person person) { throw new AssertionError("This method should not be called."); } + @Override + public boolean hasPersonName(Name name) { + throw new AssertionError("This method should not be called."); + } + @Override public void deletePerson(Person target) { throw new AssertionError("This method should not be called."); diff --git a/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java b/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java index 0f77d8295f6..78bbd67d487 100644 --- a/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java @@ -7,6 +7,8 @@ import static seedu.address.logic.commands.CommandTestUtil.showPersonAtIndex; import static seedu.address.testutil.TypicalIndexes.INDEX_FIRST_PERSON; import static seedu.address.testutil.TypicalIndexes.INDEX_SECOND_PERSON; +import static seedu.address.testutil.TypicalPersons.ALICE; +import static seedu.address.testutil.TypicalPersons.BOB; import static seedu.address.testutil.TypicalPersons.getTypicalAddressBook; import org.junit.jupiter.api.Test; @@ -16,6 +18,7 @@ import seedu.address.model.Model; import seedu.address.model.ModelManager; import seedu.address.model.UserPrefs; +import seedu.address.model.person.Name; import seedu.address.model.person.Person; /** @@ -27,9 +30,9 @@ public class DeleteCommandTest { private Model model = new ModelManager(getTypicalAddressBook(), new UserPrefs()); @Test - public void execute_validIndexUnfilteredList_success() { + public void execute_validNameUnfilteredList_success() { Person personToDelete = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased()); - DeleteCommand deleteCommand = new DeleteCommand(INDEX_FIRST_PERSON); + DeleteCommand deleteCommand = new DeleteCommand(personToDelete.getName()); String expectedMessage = String.format(DeleteCommand.MESSAGE_DELETE_PERSON_SUCCESS, personToDelete); @@ -40,19 +43,18 @@ public void execute_validIndexUnfilteredList_success() { } @Test - public void execute_invalidIndexUnfilteredList_throwsCommandException() { - Index outOfBoundIndex = Index.fromOneBased(model.getFilteredPersonList().size() + 1); - DeleteCommand deleteCommand = new DeleteCommand(outOfBoundIndex); + public void execute_invalidNameUnfilteredList_throwsCommandException() { + DeleteCommand deleteCommand = new DeleteCommand(new Name("123")); - assertCommandFailure(deleteCommand, model, Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX); + assertCommandFailure(deleteCommand, model, Messages.MESSAGE_INVALID_PERSON_DISPLAYED); } @Test - public void execute_validIndexFilteredList_success() { + public void execute_validNameFilteredList_success() { showPersonAtIndex(model, INDEX_FIRST_PERSON); Person personToDelete = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased()); - DeleteCommand deleteCommand = new DeleteCommand(INDEX_FIRST_PERSON); + DeleteCommand deleteCommand = new DeleteCommand(personToDelete.getName()); String expectedMessage = String.format(DeleteCommand.MESSAGE_DELETE_PERSON_SUCCESS, personToDelete); @@ -64,28 +66,28 @@ public void execute_validIndexFilteredList_success() { } @Test - public void execute_invalidIndexFilteredList_throwsCommandException() { + public void execute_invalidNameFilteredList_throwsCommandException() { showPersonAtIndex(model, INDEX_FIRST_PERSON); Index outOfBoundIndex = INDEX_SECOND_PERSON; // ensures that outOfBoundIndex is still in bounds of address book list assertTrue(outOfBoundIndex.getZeroBased() < model.getAddressBook().getPersonList().size()); - DeleteCommand deleteCommand = new DeleteCommand(outOfBoundIndex); + DeleteCommand deleteCommand = new DeleteCommand(new Name("123")); - assertCommandFailure(deleteCommand, model, Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX); + assertCommandFailure(deleteCommand, model, Messages.MESSAGE_INVALID_PERSON_DISPLAYED); } @Test public void equals() { - DeleteCommand deleteFirstCommand = new DeleteCommand(INDEX_FIRST_PERSON); - DeleteCommand deleteSecondCommand = new DeleteCommand(INDEX_SECOND_PERSON); + DeleteCommand deleteFirstCommand = new DeleteCommand(ALICE.getName()); + DeleteCommand deleteSecondCommand = new DeleteCommand(BOB.getName()); // same object -> returns true assertTrue(deleteFirstCommand.equals(deleteFirstCommand)); // same values -> returns true - DeleteCommand deleteFirstCommandCopy = new DeleteCommand(INDEX_FIRST_PERSON); + DeleteCommand deleteFirstCommandCopy = new DeleteCommand(ALICE.getName()); assertTrue(deleteFirstCommand.equals(deleteFirstCommandCopy)); // different types -> returns false diff --git a/src/test/java/seedu/address/logic/parser/DeleteCommandParserTest.java b/src/test/java/seedu/address/logic/parser/DeleteCommandParserTest.java index 27eaec84450..0a04c21a4ea 100644 --- a/src/test/java/seedu/address/logic/parser/DeleteCommandParserTest.java +++ b/src/test/java/seedu/address/logic/parser/DeleteCommandParserTest.java @@ -3,7 +3,7 @@ import static seedu.address.commons.core.Messages.MESSAGE_INVALID_COMMAND_FORMAT; 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.TypicalPersons.ALICE; import org.junit.jupiter.api.Test; @@ -22,11 +22,11 @@ public class DeleteCommandParserTest { @Test public void parse_validArgs_returnsDeleteCommand() { - assertParseSuccess(parser, "1", new DeleteCommand(INDEX_FIRST_PERSON)); + assertParseSuccess(parser, "Alice Pauline", new DeleteCommand(ALICE.getName())); } @Test public void parse_invalidArgs_throwsParseException() { - assertParseFailure(parser, "a", String.format(MESSAGE_INVALID_COMMAND_FORMAT, DeleteCommand.MESSAGE_USAGE)); + assertParseFailure(parser, "", String.format(MESSAGE_INVALID_COMMAND_FORMAT, DeleteCommand.MESSAGE_USAGE)); } } diff --git a/src/test/java/seedu/address/model/person/PersonTest.java b/src/test/java/seedu/address/model/person/PersonTest.java index da23d83e5fd..a51c1b5b582 100644 --- a/src/test/java/seedu/address/model/person/PersonTest.java +++ b/src/test/java/seedu/address/model/person/PersonTest.java @@ -30,9 +30,9 @@ public void isSamePerson() { // null -> returns false assertFalse(ALICE.isSamePerson(null)); - // different phone and email -> returns false + // different phone and email but same name -> returns true Person editedAlice = new PersonBuilder(ALICE).withPhone(VALID_PHONE_BOB).withEmail(VALID_EMAIL_BOB).build(); - assertFalse(ALICE.isSamePerson(editedAlice)); + assertTrue(ALICE.isSamePerson(editedAlice)); // different name -> returns false editedAlice = new PersonBuilder(ALICE).withName(VALID_NAME_BOB).build(); diff --git a/src/test/java/seedu/address/model/person/UniquePersonListTest.java b/src/test/java/seedu/address/model/person/UniquePersonListTest.java index 5df427de2c0..41a923d3a1f 100644 --- a/src/test/java/seedu/address/model/person/UniquePersonListTest.java +++ b/src/test/java/seedu/address/model/person/UniquePersonListTest.java @@ -22,9 +22,14 @@ public class UniquePersonListTest { private final UniquePersonList uniquePersonList = new UniquePersonList(); + @Test + public void contains_nullName_throwsNullPointerException() { + assertThrows(NullPointerException.class, () -> uniquePersonList.contains((Name) null)); + } + @Test public void contains_nullPerson_throwsNullPointerException() { - assertThrows(NullPointerException.class, () -> uniquePersonList.contains(null)); + assertThrows(NullPointerException.class, () -> uniquePersonList.contains((Person) null)); } @Test diff --git a/src/test/java/seedu/address/testutil/PersonBuilder.java b/src/test/java/seedu/address/testutil/PersonBuilder.java index fba5147d07f..bf51e0e390c 100644 --- a/src/test/java/seedu/address/testutil/PersonBuilder.java +++ b/src/test/java/seedu/address/testutil/PersonBuilder.java @@ -15,10 +15,9 @@ */ public class PersonBuilder { - public static final String DEFAULT_NAME = "Alice Pauline"; + public static final String DEFAULT_NAME = "Sally Toh"; public static final String DEFAULT_PHONE = "85355255"; - public static final String DEFAULT_EMAIL = "alice@gmail.com"; - public static final String DEFAULT_ADDRESS = "123, Jurong West Ave 6, #08-111"; + public static final String DEFAULT_EMAIL = "sally@yahoo.com"; private Name name; private Phone phone;