From 000dbd1645d4c193088a2f94429642fca5c1e203 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Wed, 8 Aug 2018 17:12:26 +0800 Subject: [PATCH 01/11] Remove redundant throws clauses These redundant throws clauses were identified using IntelliJ's Analyze->Inspect Code feature, and analyzing the whole project. --- src/main/java/seedu/address/commons/util/JsonUtil.java | 2 +- .../java/seedu/address/storage/JsonUserPrefsStorage.java | 2 +- .../java/seedu/address/logic/commands/AddCommandTest.java | 8 +++----- .../seedu/address/logic/commands/DeleteCommandTest.java | 4 ++-- .../seedu/address/logic/commands/EditCommandTest.java | 2 +- .../seedu/address/storage/XmlAddressBookStorageTest.java | 2 +- src/test/java/systemtests/AddressBookSystemTest.java | 2 +- 7 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/main/java/seedu/address/commons/util/JsonUtil.java b/src/main/java/seedu/address/commons/util/JsonUtil.java index 0b13f5180370..8ecd614f550a 100644 --- a/src/main/java/seedu/address/commons/util/JsonUtil.java +++ b/src/main/java/seedu/address/commons/util/JsonUtil.java @@ -120,7 +120,7 @@ protected LevelDeserializer(Class vc) { } @Override - protected Level _deserialize(String value, DeserializationContext ctxt) throws IOException { + protected Level _deserialize(String value, DeserializationContext ctxt) { return getLoggingLevel(value); } diff --git a/src/main/java/seedu/address/storage/JsonUserPrefsStorage.java b/src/main/java/seedu/address/storage/JsonUserPrefsStorage.java index 498fe3c4e9a0..2ab927023cc4 100644 --- a/src/main/java/seedu/address/storage/JsonUserPrefsStorage.java +++ b/src/main/java/seedu/address/storage/JsonUserPrefsStorage.java @@ -25,7 +25,7 @@ public Path getUserPrefsFilePath() { } @Override - public Optional readUserPrefs() throws DataConversionException, IOException { + public Optional readUserPrefs() throws DataConversionException { return readUserPrefs(filePath); } diff --git a/src/test/java/seedu/address/logic/commands/AddCommandTest.java b/src/test/java/seedu/address/logic/commands/AddCommandTest.java index 571348cb923d..c3739fbf3465 100644 --- a/src/test/java/seedu/address/logic/commands/AddCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/AddCommandTest.java @@ -21,7 +21,6 @@ import seedu.address.model.ReadOnlyAddressBook; import seedu.address.model.person.Person; import seedu.address.model.person.exceptions.DuplicatePersonException; -import seedu.address.model.person.exceptions.PersonNotFoundException; import seedu.address.testutil.PersonBuilder; public class AddCommandTest { @@ -110,13 +109,12 @@ public ReadOnlyAddressBook getAddressBook() { } @Override - public void deletePerson(Person target) throws PersonNotFoundException { + public void deletePerson(Person target) { throw new AssertionError("This method should not be called."); } @Override - public void updatePerson(Person target, Person editedPerson) - throws DuplicatePersonException { + public void updatePerson(Person target, Person editedPerson) { throw new AssertionError("This method should not be called."); } @@ -178,7 +176,7 @@ private class ModelStubAcceptingPersonAdded extends ModelStub { final ArrayList personsAdded = new ArrayList<>(); @Override - public void addPerson(Person person) throws DuplicatePersonException { + public void addPerson(Person person) { requireNonNull(person); personsAdded.add(person); } diff --git a/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java b/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java index add7a2ebe5d4..dab1a682a43e 100644 --- a/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java @@ -45,7 +45,7 @@ public void execute_validIndexUnfilteredList_success() throws Exception { } @Test - public void execute_invalidIndexUnfilteredList_throwsCommandException() throws Exception { + public void execute_invalidIndexUnfilteredList_throwsCommandException() { Index outOfBoundIndex = Index.fromOneBased(model.getFilteredPersonList().size() + 1); DeleteCommand deleteCommand = prepareCommand(outOfBoundIndex); @@ -152,7 +152,7 @@ public void executeUndoRedo_validIndexFilteredList_samePersonDeleted() throws Ex } @Test - public void equals() throws Exception { + public void equals() { DeleteCommand deleteFirstCommand = prepareCommand(INDEX_FIRST_PERSON); DeleteCommand deleteSecondCommand = prepareCommand(INDEX_SECOND_PERSON); diff --git a/src/test/java/seedu/address/logic/commands/EditCommandTest.java b/src/test/java/seedu/address/logic/commands/EditCommandTest.java index 7057f8d2afa5..06612527d891 100644 --- a/src/test/java/seedu/address/logic/commands/EditCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/EditCommandTest.java @@ -228,7 +228,7 @@ public void executeUndoRedo_validIndexFilteredList_samePersonEdited() throws Exc } @Test - public void equals() throws Exception { + public void equals() { final EditCommand standardCommand = prepareCommand(INDEX_FIRST_PERSON, DESC_AMY); // same values -> returns true diff --git a/src/test/java/seedu/address/storage/XmlAddressBookStorageTest.java b/src/test/java/seedu/address/storage/XmlAddressBookStorageTest.java index 7ed10523477b..484bd43cc0fd 100644 --- a/src/test/java/seedu/address/storage/XmlAddressBookStorageTest.java +++ b/src/test/java/seedu/address/storage/XmlAddressBookStorageTest.java @@ -118,7 +118,7 @@ private void saveAddressBook(ReadOnlyAddressBook addressBook, String filePath) { } @Test - public void saveAddressBook_nullFilePath_throwsNullPointerException() throws IOException { + public void saveAddressBook_nullFilePath_throwsNullPointerException() { thrown.expect(NullPointerException.class); saveAddressBook(new AddressBook(), null); } diff --git a/src/test/java/systemtests/AddressBookSystemTest.java b/src/test/java/systemtests/AddressBookSystemTest.java index 986c82360359..53e13959d136 100644 --- a/src/test/java/systemtests/AddressBookSystemTest.java +++ b/src/test/java/systemtests/AddressBookSystemTest.java @@ -76,7 +76,7 @@ public void setUp() { } @After - public void tearDown() throws Exception { + public void tearDown() { setupHelper.tearDownStage(); EventsCenter.clearSubscribers(); } From ac42c32ff5c7eb604a751dc35964eb7388bf8bb4 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Mon, 6 Aug 2018 18:57:26 +0800 Subject: [PATCH 02/11] model: expose hasPerson(Person) functionality In the next few commits, we are going to be converting DuplicatePersonException and PersonNotFoundException into runtime exceptions. To do that, we need to be able to query the model to determine if a person exists in the address book. This functionality is already available in `UniquePersonList#contains(Person)`, but was simply not exposed. Let's expose it. --- .../java/seedu/address/model/AddressBook.java | 8 ++++++ src/main/java/seedu/address/model/Model.java | 5 ++++ .../seedu/address/model/ModelManager.java | 6 +++++ .../logic/commands/AddCommandTest.java | 5 ++++ .../seedu/address/model/AddressBookTest.java | 27 +++++++++++++++++++ .../seedu/address/model/ModelManagerTest.java | 22 +++++++++++++-- 6 files changed, 71 insertions(+), 2 deletions(-) diff --git a/src/main/java/seedu/address/model/AddressBook.java b/src/main/java/seedu/address/model/AddressBook.java index f5124edcf466..bdcd38ef0c0d 100644 --- a/src/main/java/seedu/address/model/AddressBook.java +++ b/src/main/java/seedu/address/model/AddressBook.java @@ -60,6 +60,14 @@ public void resetData(ReadOnlyAddressBook newData) { //// person-level operations + /** + * Returns true if a person with the same identity as {@code person} exists in the address book. + */ + public boolean hasPerson(Person person) { + requireNonNull(person); + return persons.contains(person); + } + /** * Adds a person to the address book. * diff --git a/src/main/java/seedu/address/model/Model.java b/src/main/java/seedu/address/model/Model.java index 89574cbf09b0..cd84057ee122 100644 --- a/src/main/java/seedu/address/model/Model.java +++ b/src/main/java/seedu/address/model/Model.java @@ -20,6 +20,11 @@ public interface Model { /** Returns the AddressBook */ ReadOnlyAddressBook getAddressBook(); + /** + * Returns true if a person with the same identity as {@code person} exists in the address book. + */ + boolean hasPerson(Person person); + /** Deletes the given person. */ void deletePerson(Person target) throws PersonNotFoundException; diff --git a/src/main/java/seedu/address/model/ModelManager.java b/src/main/java/seedu/address/model/ModelManager.java index c2b56ffd4b90..1994ed254504 100644 --- a/src/main/java/seedu/address/model/ModelManager.java +++ b/src/main/java/seedu/address/model/ModelManager.java @@ -59,6 +59,12 @@ private void indicateAddressBookChanged() { raise(new AddressBookChangedEvent(versionedAddressBook)); } + @Override + public synchronized boolean hasPerson(Person person) { + requireNonNull(person); + return versionedAddressBook.hasPerson(person); + } + @Override public synchronized void deletePerson(Person target) throws PersonNotFoundException { versionedAddressBook.removePerson(target); diff --git a/src/test/java/seedu/address/logic/commands/AddCommandTest.java b/src/test/java/seedu/address/logic/commands/AddCommandTest.java index c3739fbf3465..ab75039f2190 100644 --- a/src/test/java/seedu/address/logic/commands/AddCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/AddCommandTest.java @@ -108,6 +108,11 @@ public ReadOnlyAddressBook getAddressBook() { throw new AssertionError("This method should not be called."); } + @Override + public boolean hasPerson(Person person) { + 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/model/AddressBookTest.java b/src/test/java/seedu/address/model/AddressBookTest.java index 2189d5d860d5..7ecbd5a47686 100644 --- a/src/test/java/seedu/address/model/AddressBookTest.java +++ b/src/test/java/seedu/address/model/AddressBookTest.java @@ -1,6 +1,8 @@ package seedu.address.model; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static seedu.address.logic.commands.CommandTestUtil.VALID_ADDRESS_BOB; import static seedu.address.logic.commands.CommandTestUtil.VALID_TAG_HUSBAND; import static seedu.address.testutil.TypicalPersons.ALICE; @@ -57,6 +59,31 @@ public void resetData_withDuplicatePersons_throwsAssertionError() { addressBook.resetData(newData); } + @Test + public void hasPerson_nullPerson_throwsNullPointerException() { + thrown.expect(NullPointerException.class); + addressBook.hasPerson(null); + } + + @Test + public void hasPerson_personNotInAddressBook_returnsFalse() { + assertFalse(addressBook.hasPerson(ALICE)); + } + + @Test + public void hasPerson_personInAddressBook_returnsTrue() throws Exception { + addressBook.addPerson(ALICE); + assertTrue(addressBook.hasPerson(ALICE)); + } + + @Test + public void hasPerson_personWithSameIdentityFieldsInAddressBook_returnsTrue() throws Exception { + addressBook.addPerson(ALICE); + Person editedAlice = new PersonBuilder(ALICE).withAddress(VALID_ADDRESS_BOB).withTags(VALID_TAG_HUSBAND) + .build(); + assertTrue(addressBook.hasPerson(editedAlice)); + } + @Test public void getPersonList_modifyList_throwsUnsupportedOperationException() { thrown.expect(UnsupportedOperationException.class); diff --git a/src/test/java/seedu/address/model/ModelManagerTest.java b/src/test/java/seedu/address/model/ModelManagerTest.java index 59ce1b83693a..2f5f39669315 100644 --- a/src/test/java/seedu/address/model/ModelManagerTest.java +++ b/src/test/java/seedu/address/model/ModelManagerTest.java @@ -19,9 +19,27 @@ public class ModelManagerTest { @Rule public ExpectedException thrown = ExpectedException.none(); + private ModelManager modelManager = new ModelManager(); + + @Test + public void hasPerson_nullPerson_throwsNullPointerException() { + thrown.expect(NullPointerException.class); + modelManager.hasPerson(null); + } + + @Test + public void hasPerson_personNotInAddressBook_returnsFalse() { + assertFalse(modelManager.hasPerson(ALICE)); + } + + @Test + public void hasPerson_personInAddressBook_returnsTrue() throws Exception { + modelManager.addPerson(ALICE); + assertTrue(modelManager.hasPerson(ALICE)); + } + @Test public void getFilteredPersonList_modifyList_throwsUnsupportedOperationException() { - ModelManager modelManager = new ModelManager(); thrown.expect(UnsupportedOperationException.class); modelManager.getFilteredPersonList().remove(0); } @@ -33,7 +51,7 @@ public void equals() { UserPrefs userPrefs = new UserPrefs(); // same values -> returns true - ModelManager modelManager = new ModelManager(addressBook, userPrefs); + modelManager = new ModelManager(addressBook, userPrefs); ModelManager modelManagerCopy = new ModelManager(addressBook, userPrefs); assertTrue(modelManager.equals(modelManagerCopy)); From 5e14e423d39aba11f5ccf5697e30ffa3c72823ca Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 7 Aug 2018 22:39:15 +0800 Subject: [PATCH 03/11] AddCommand: explicitly perform duplicate person check In the next few commits, we will convert DuplicatePersonException into a runtime exception. In preparation for that, let's teach AddCommand to not depend on catching DuplicatePersonException for its control flow. Since DuplicatePersonException is still a checked exception, we need to add an ugly `throw new AssertionError("should not happen")`, but this will be removed in later commits once we convert DuplicatePersonException into a runtime exception. --- .../address/logic/commands/AddCommand.java | 7 +++++- .../logic/commands/AddCommandTest.java | 25 +++++++++++++------ 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/main/java/seedu/address/logic/commands/AddCommand.java b/src/main/java/seedu/address/logic/commands/AddCommand.java index 77d5ead50c6d..7e611bee6a05 100644 --- a/src/main/java/seedu/address/logic/commands/AddCommand.java +++ b/src/main/java/seedu/address/logic/commands/AddCommand.java @@ -49,12 +49,17 @@ public AddCommand(Person person) { @Override public CommandResult execute() throws CommandException { requireNonNull(model); + + if (model.hasPerson(toAdd)) { + throw new CommandException(MESSAGE_DUPLICATE_PERSON); + } + try { model.addPerson(toAdd); model.commitAddressBook(); return new CommandResult(String.format(MESSAGE_SUCCESS, toAdd)); } catch (DuplicatePersonException e) { - throw new CommandException(MESSAGE_DUPLICATE_PERSON, e); + throw new AssertionError("should not happen", e); } } diff --git a/src/test/java/seedu/address/logic/commands/AddCommandTest.java b/src/test/java/seedu/address/logic/commands/AddCommandTest.java index ab75039f2190..3ab744c4e4e8 100644 --- a/src/test/java/seedu/address/logic/commands/AddCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/AddCommandTest.java @@ -47,8 +47,8 @@ public void execute_personAcceptedByModel_addSuccessful() throws Exception { @Test public void execute_duplicatePerson_throwsCommandException() throws Exception { - ModelStub modelStub = new ModelStubThrowingDuplicatePersonException(); Person validPerson = new PersonBuilder().build(); + ModelStub modelStub = new ModelStubWithPerson(validPerson); thrown.expect(CommandException.class); thrown.expectMessage(AddCommand.MESSAGE_DUPLICATE_PERSON); @@ -160,17 +160,20 @@ public void commitAddressBook() { } /** - * A Model stub that always throw a DuplicatePersonException when trying to add a person. + * A Model stub that contains a single person. */ - private class ModelStubThrowingDuplicatePersonException extends ModelStub { - @Override - public void addPerson(Person person) throws DuplicatePersonException { - throw new DuplicatePersonException(); + private class ModelStubWithPerson extends ModelStub { + private final Person person; + + ModelStubWithPerson(Person person) { + requireNonNull(person); + this.person = person; } @Override - public ReadOnlyAddressBook getAddressBook() { - return new AddressBook(); + public boolean hasPerson(Person person) { + requireNonNull(person); + return this.person.isSamePerson(person); } } @@ -180,6 +183,12 @@ public ReadOnlyAddressBook getAddressBook() { private class ModelStubAcceptingPersonAdded extends ModelStub { final ArrayList personsAdded = new ArrayList<>(); + @Override + public boolean hasPerson(Person person) { + requireNonNull(person); + return personsAdded.stream().anyMatch(person::isSamePerson); + } + @Override public void addPerson(Person person) { requireNonNull(person); From e66d31c483a0fcaeb953287e2af56962284c2841 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 7 Aug 2018 22:57:18 +0800 Subject: [PATCH 04/11] EditCommand: explicitly perform duplicate person check In the next few commits, we will convert DuplicatePersonException into a runtime exception. In preparation for that, let's teach EditCommand to not depend on catching DuplicatePersonException for its control flow. Since DuplicatePersonException is still a checked exception, we need to add an ugly `throw new AssertionError("should not happen")`, but this will be removed in later commits once we convert DuplicatePersonException into a runtime exception. --- src/main/java/seedu/address/logic/commands/EditCommand.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/seedu/address/logic/commands/EditCommand.java b/src/main/java/seedu/address/logic/commands/EditCommand.java index f0798f3fa00b..0a845a18ba3b 100644 --- a/src/main/java/seedu/address/logic/commands/EditCommand.java +++ b/src/main/java/seedu/address/logic/commands/EditCommand.java @@ -77,10 +77,14 @@ public CommandResult execute() throws CommandException { Person personToEdit = lastShownList.get(index.getZeroBased()); Person editedPerson = createEditedPerson(personToEdit, editPersonDescriptor); + if (!personToEdit.isSamePerson(editedPerson) && model.hasPerson(editedPerson)) { + throw new CommandException(MESSAGE_DUPLICATE_PERSON); + } + try { model.updatePerson(personToEdit, editedPerson); } catch (DuplicatePersonException dpe) { - throw new CommandException(MESSAGE_DUPLICATE_PERSON, dpe); + throw new AssertionError("should not happen", dpe); } catch (PersonNotFoundException pnfe) { throw new AssertionError("The target person cannot be missing", pnfe); } From 6817bd359355bc3c1fbee1f75b51de60afc4c97e Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 7 Aug 2018 22:24:17 +0800 Subject: [PATCH 05/11] model: make DuplicatePersonException a RuntimeException DuplicatePersonException is a checked exception. It is thrown by methods such as `UniquePersonList#add(Person)`, `UniquePersonList#setPerson(Person, Person)` etc. to signify that certain preconditions were not met -- namely that the operation would cause the "no duplicate persons in address book/UniquePersonList" constraint to be violated. Using checked exceptions for such a purpose is slightly useful in that it will force callers to handle the case where the above preconditions are not met, such as when the methods are called with invalid user input. However, it imposes a HUGE cost on callers where the preconditions are already known to be met (e.g. in test code, or when the user input has already been validated before hand). In such a case, callers are forced to add try-catch blocks around the method call even if they know that the exception will never be thrown, bloating up the code. It is also impossible to test the catch blocks as well since correct code will ensure that the precondition holds and thus the exception will never be thrown, leading to reduced code coverage. Checked exceptions also don't work very well with the Java Streams API, since the API doesn't accept lambdas which could throw checked exceptions. In AB-4, the amount of code which benefits from DuplicatePersonException being a checked exception is much smaller than the amount of code which is negatively impacted. As such, let's make the tradeoff in the other direction, by making DuplicatePersonException a RuntimeException. New callers _could_ forget to check that the preconditions hold before calling the methods in question (although test cases should catch that), but this is balanced out by the huge benefit of having more concise and testable code. --- .../address/logic/commands/AddCommand.java | 12 +++------- .../address/logic/commands/EditCommand.java | 3 --- .../java/seedu/address/model/AddressBook.java | 24 ++++++++----------- src/main/java/seedu/address/model/Model.java | 14 +++++------ .../seedu/address/model/ModelManager.java | 6 ++--- .../model/person/UniquePersonList.java | 16 +++++++------ .../exceptions/DuplicatePersonException.java | 4 +--- .../address/model/util/SampleDataUtil.java | 13 ++++------ .../commands/AddCommandIntegrationTest.java | 2 +- .../logic/commands/AddCommandTest.java | 3 +-- .../seedu/address/model/AddressBookTest.java | 9 +++---- .../seedu/address/model/ModelManagerTest.java | 2 +- .../address/testutil/AddressBookBuilder.java | 7 +----- .../address/testutil/TypicalPersons.java | 7 +----- .../systemtests/AddCommandSystemTest.java | 9 ++----- .../systemtests/EditCommandSystemTest.java | 6 ++--- 16 files changed, 50 insertions(+), 87 deletions(-) diff --git a/src/main/java/seedu/address/logic/commands/AddCommand.java b/src/main/java/seedu/address/logic/commands/AddCommand.java index 7e611bee6a05..a5315c72e6e3 100644 --- a/src/main/java/seedu/address/logic/commands/AddCommand.java +++ b/src/main/java/seedu/address/logic/commands/AddCommand.java @@ -9,7 +9,6 @@ import seedu.address.logic.commands.exceptions.CommandException; import seedu.address.model.person.Person; -import seedu.address.model.person.exceptions.DuplicatePersonException; /** * Adds a person to the address book. @@ -54,14 +53,9 @@ public CommandResult execute() throws CommandException { throw new CommandException(MESSAGE_DUPLICATE_PERSON); } - try { - model.addPerson(toAdd); - model.commitAddressBook(); - return new CommandResult(String.format(MESSAGE_SUCCESS, toAdd)); - } catch (DuplicatePersonException e) { - throw new AssertionError("should not happen", e); - } - + model.addPerson(toAdd); + model.commitAddressBook(); + return new CommandResult(String.format(MESSAGE_SUCCESS, toAdd)); } @Override diff --git a/src/main/java/seedu/address/logic/commands/EditCommand.java b/src/main/java/seedu/address/logic/commands/EditCommand.java index 0a845a18ba3b..5d79efe7e246 100644 --- a/src/main/java/seedu/address/logic/commands/EditCommand.java +++ b/src/main/java/seedu/address/logic/commands/EditCommand.java @@ -23,7 +23,6 @@ import seedu.address.model.person.Name; import seedu.address.model.person.Person; import seedu.address.model.person.Phone; -import seedu.address.model.person.exceptions.DuplicatePersonException; import seedu.address.model.person.exceptions.PersonNotFoundException; import seedu.address.model.tag.Tag; @@ -83,8 +82,6 @@ public CommandResult execute() throws CommandException { try { model.updatePerson(personToEdit, editedPerson); - } catch (DuplicatePersonException dpe) { - throw new AssertionError("should not happen", dpe); } catch (PersonNotFoundException pnfe) { throw new AssertionError("The target person cannot be missing", pnfe); } diff --git a/src/main/java/seedu/address/model/AddressBook.java b/src/main/java/seedu/address/model/AddressBook.java index bdcd38ef0c0d..2c4c06ddbb28 100644 --- a/src/main/java/seedu/address/model/AddressBook.java +++ b/src/main/java/seedu/address/model/AddressBook.java @@ -7,7 +7,6 @@ import javafx.collections.ObservableList; import seedu.address.model.person.Person; import seedu.address.model.person.UniquePersonList; -import seedu.address.model.person.exceptions.DuplicatePersonException; import seedu.address.model.person.exceptions.PersonNotFoundException; /** @@ -41,7 +40,11 @@ public AddressBook(ReadOnlyAddressBook toBeCopied) { //// list overwrite operations - public void setPersons(List persons) throws DuplicatePersonException { + /** + * Replaces the contents of the person list with {@code persons}. + * {@code persons} must not contain duplicate persons. + */ + public void setPersons(List persons) { this.persons.setPersons(persons); } @@ -51,11 +54,7 @@ public void setPersons(List persons) throws DuplicatePersonException { public void resetData(ReadOnlyAddressBook newData) { requireNonNull(newData); - try { - setPersons(newData.getPersonList()); - } catch (DuplicatePersonException e) { - throw new AssertionError("AddressBooks should not have duplicate persons", e); - } + setPersons(newData.getPersonList()); } //// person-level operations @@ -70,22 +69,19 @@ public boolean hasPerson(Person person) { /** * Adds a person to the address book. - * - * @throws DuplicatePersonException if an equivalent person already exists. + * The person must not already exist in the address book. */ - public void addPerson(Person p) throws DuplicatePersonException { + public void addPerson(Person p) { persons.add(p); } /** * Replaces the given person {@code target} in the list with {@code editedPerson}. + * The person identity of {@code editedPerson} must not be the same as another existing person in the address book. * - * @throws DuplicatePersonException if updating the person's details causes the person to be equivalent to - * another existing person in the list. * @throws PersonNotFoundException if {@code target} could not be found in the list. */ - public void updatePerson(Person target, Person editedPerson) - throws DuplicatePersonException, PersonNotFoundException { + public void updatePerson(Person target, Person editedPerson) throws PersonNotFoundException { requireNonNull(editedPerson); persons.setPerson(target, editedPerson); diff --git a/src/main/java/seedu/address/model/Model.java b/src/main/java/seedu/address/model/Model.java index cd84057ee122..fe6ad8333305 100644 --- a/src/main/java/seedu/address/model/Model.java +++ b/src/main/java/seedu/address/model/Model.java @@ -4,7 +4,6 @@ import javafx.collections.ObservableList; import seedu.address.model.person.Person; -import seedu.address.model.person.exceptions.DuplicatePersonException; import seedu.address.model.person.exceptions.PersonNotFoundException; /** @@ -28,18 +27,19 @@ public interface Model { /** Deletes the given person. */ void deletePerson(Person target) throws PersonNotFoundException; - /** Adds the given person */ - void addPerson(Person person) throws DuplicatePersonException; + /** + * Adds the given person. + * {@code person} must not already exist in the address book. + */ + void addPerson(Person person); /** * Replaces the given person {@code target} with {@code editedPerson}. + * The person identity of {@code editedPerson} must not be the same as another existing person in the address book. * - * @throws DuplicatePersonException if updating the person's details causes the person to be equivalent to - * another existing person in the list. * @throws PersonNotFoundException if {@code target} could not be found in the list. */ - void updatePerson(Person target, Person editedPerson) - throws DuplicatePersonException, PersonNotFoundException; + void updatePerson(Person target, Person editedPerson) throws PersonNotFoundException; /** Returns an unmodifiable view of the filtered person list */ ObservableList getFilteredPersonList(); diff --git a/src/main/java/seedu/address/model/ModelManager.java b/src/main/java/seedu/address/model/ModelManager.java index 1994ed254504..913cb6864377 100644 --- a/src/main/java/seedu/address/model/ModelManager.java +++ b/src/main/java/seedu/address/model/ModelManager.java @@ -13,7 +13,6 @@ import seedu.address.commons.core.LogsCenter; import seedu.address.commons.events.model.AddressBookChangedEvent; import seedu.address.model.person.Person; -import seedu.address.model.person.exceptions.DuplicatePersonException; import seedu.address.model.person.exceptions.PersonNotFoundException; /** @@ -72,15 +71,14 @@ public synchronized void deletePerson(Person target) throws PersonNotFoundExcept } @Override - public synchronized void addPerson(Person person) throws DuplicatePersonException { + public synchronized void addPerson(Person person) { versionedAddressBook.addPerson(person); updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS); indicateAddressBookChanged(); } @Override - public void updatePerson(Person target, Person editedPerson) - throws DuplicatePersonException, PersonNotFoundException { + public void updatePerson(Person target, Person editedPerson) throws PersonNotFoundException { requireAllNonNull(target, editedPerson); versionedAddressBook.updatePerson(target, editedPerson); diff --git a/src/main/java/seedu/address/model/person/UniquePersonList.java b/src/main/java/seedu/address/model/person/UniquePersonList.java index 7def02f56a6e..71dc1043651e 100644 --- a/src/main/java/seedu/address/model/person/UniquePersonList.java +++ b/src/main/java/seedu/address/model/person/UniquePersonList.java @@ -36,10 +36,9 @@ public boolean contains(Person toCheck) { /** * Adds a person to the list. - * - * @throws DuplicatePersonException if the person to add is a duplicate of an existing person in the list. + * The person must not already exist in the list. */ - public void add(Person toAdd) throws DuplicatePersonException { + public void add(Person toAdd) { requireNonNull(toAdd); if (contains(toAdd)) { throw new DuplicatePersonException(); @@ -49,12 +48,11 @@ public void add(Person toAdd) throws DuplicatePersonException { /** * Replaces the person {@code target} in the list with {@code editedPerson}. + * The person identity of {@code editedPerson} must not be the same as another existing person in the list. * - * @throws DuplicatePersonException if the replacement is equivalent to another existing person in the list. * @throws PersonNotFoundException if {@code target} could not be found in the list. */ - public void setPerson(Person target, Person editedPerson) - throws DuplicatePersonException, PersonNotFoundException { + public void setPerson(Person target, Person editedPerson) throws PersonNotFoundException { requireNonNull(editedPerson); int index = internalList.indexOf(target); @@ -85,7 +83,11 @@ public void setPersons(UniquePersonList replacement) { this.internalList.setAll(replacement.internalList); } - public void setPersons(List persons) throws DuplicatePersonException { + /** + * Replaces the contents of this list with {@code persons}. + * {@code persons} must not contain duplicate persons. + */ + public void setPersons(List persons) { requireAllNonNull(persons); if (!personsAreUnique(persons)) { throw new DuplicatePersonException(); diff --git a/src/main/java/seedu/address/model/person/exceptions/DuplicatePersonException.java b/src/main/java/seedu/address/model/person/exceptions/DuplicatePersonException.java index cad32d6ee545..d7290f594423 100644 --- a/src/main/java/seedu/address/model/person/exceptions/DuplicatePersonException.java +++ b/src/main/java/seedu/address/model/person/exceptions/DuplicatePersonException.java @@ -1,12 +1,10 @@ package seedu.address.model.person.exceptions; -import seedu.address.commons.exceptions.DuplicateDataException; - /** * Signals that the operation will result in duplicate Persons (Persons are considered duplicates if they have the same * identity). */ -public class DuplicatePersonException extends DuplicateDataException { +public class DuplicatePersonException extends RuntimeException { public DuplicatePersonException() { super("Operation would result in duplicate persons"); } diff --git a/src/main/java/seedu/address/model/util/SampleDataUtil.java b/src/main/java/seedu/address/model/util/SampleDataUtil.java index 31d4034a3845..1806da4facfa 100644 --- a/src/main/java/seedu/address/model/util/SampleDataUtil.java +++ b/src/main/java/seedu/address/model/util/SampleDataUtil.java @@ -11,7 +11,6 @@ import seedu.address.model.person.Name; import seedu.address.model.person.Person; import seedu.address.model.person.Phone; -import seedu.address.model.person.exceptions.DuplicatePersonException; import seedu.address.model.tag.Tag; /** @@ -42,15 +41,11 @@ public static Person[] getSamplePersons() { } public static ReadOnlyAddressBook getSampleAddressBook() { - try { - AddressBook sampleAb = new AddressBook(); - for (Person samplePerson : getSamplePersons()) { - sampleAb.addPerson(samplePerson); - } - return sampleAb; - } catch (DuplicatePersonException e) { - throw new AssertionError("sample data cannot contain duplicate persons", e); + AddressBook sampleAb = new AddressBook(); + for (Person samplePerson : getSamplePersons()) { + sampleAb.addPerson(samplePerson); } + return sampleAb; } /** diff --git a/src/test/java/seedu/address/logic/commands/AddCommandIntegrationTest.java b/src/test/java/seedu/address/logic/commands/AddCommandIntegrationTest.java index 762bce019f38..b42316794b4f 100644 --- a/src/test/java/seedu/address/logic/commands/AddCommandIntegrationTest.java +++ b/src/test/java/seedu/address/logic/commands/AddCommandIntegrationTest.java @@ -27,7 +27,7 @@ public void setUp() { } @Test - public void execute_newPerson_success() throws Exception { + public void execute_newPerson_success() { Person validPerson = new PersonBuilder().build(); Model expectedModel = new ModelManager(model.getAddressBook(), new UserPrefs()); diff --git a/src/test/java/seedu/address/logic/commands/AddCommandTest.java b/src/test/java/seedu/address/logic/commands/AddCommandTest.java index 3ab744c4e4e8..03e742d13c30 100644 --- a/src/test/java/seedu/address/logic/commands/AddCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/AddCommandTest.java @@ -20,7 +20,6 @@ import seedu.address.model.Model; import seedu.address.model.ReadOnlyAddressBook; import seedu.address.model.person.Person; -import seedu.address.model.person.exceptions.DuplicatePersonException; import seedu.address.testutil.PersonBuilder; public class AddCommandTest { @@ -94,7 +93,7 @@ private AddCommand getAddCommandForPerson(Person person, Model model) { */ private class ModelStub implements Model { @Override - public void addPerson(Person person) throws DuplicatePersonException { + public void addPerson(Person person) { throw new AssertionError("This method should not be called."); } diff --git a/src/test/java/seedu/address/model/AddressBookTest.java b/src/test/java/seedu/address/model/AddressBookTest.java index 7ecbd5a47686..0d33cff49ab1 100644 --- a/src/test/java/seedu/address/model/AddressBookTest.java +++ b/src/test/java/seedu/address/model/AddressBookTest.java @@ -20,6 +20,7 @@ import javafx.collections.FXCollections; import javafx.collections.ObservableList; import seedu.address.model.person.Person; +import seedu.address.model.person.exceptions.DuplicatePersonException; import seedu.address.testutil.PersonBuilder; public class AddressBookTest { @@ -48,14 +49,14 @@ public void resetData_withValidReadOnlyAddressBook_replacesData() { } @Test - public void resetData_withDuplicatePersons_throwsAssertionError() { + public void resetData_withDuplicatePersons_throwsDuplicatePersonException() { // Two persons with the same identity fields Person editedAlice = new PersonBuilder(ALICE).withAddress(VALID_ADDRESS_BOB).withTags(VALID_TAG_HUSBAND) .build(); List newPersons = Arrays.asList(ALICE, editedAlice); AddressBookStub newData = new AddressBookStub(newPersons); - thrown.expect(AssertionError.class); + thrown.expect(DuplicatePersonException.class); addressBook.resetData(newData); } @@ -71,13 +72,13 @@ public void hasPerson_personNotInAddressBook_returnsFalse() { } @Test - public void hasPerson_personInAddressBook_returnsTrue() throws Exception { + public void hasPerson_personInAddressBook_returnsTrue() { addressBook.addPerson(ALICE); assertTrue(addressBook.hasPerson(ALICE)); } @Test - public void hasPerson_personWithSameIdentityFieldsInAddressBook_returnsTrue() throws Exception { + public void hasPerson_personWithSameIdentityFieldsInAddressBook_returnsTrue() { addressBook.addPerson(ALICE); Person editedAlice = new PersonBuilder(ALICE).withAddress(VALID_ADDRESS_BOB).withTags(VALID_TAG_HUSBAND) .build(); diff --git a/src/test/java/seedu/address/model/ModelManagerTest.java b/src/test/java/seedu/address/model/ModelManagerTest.java index 2f5f39669315..bb173e4af3ee 100644 --- a/src/test/java/seedu/address/model/ModelManagerTest.java +++ b/src/test/java/seedu/address/model/ModelManagerTest.java @@ -33,7 +33,7 @@ public void hasPerson_personNotInAddressBook_returnsFalse() { } @Test - public void hasPerson_personInAddressBook_returnsTrue() throws Exception { + public void hasPerson_personInAddressBook_returnsTrue() { modelManager.addPerson(ALICE); assertTrue(modelManager.hasPerson(ALICE)); } diff --git a/src/test/java/seedu/address/testutil/AddressBookBuilder.java b/src/test/java/seedu/address/testutil/AddressBookBuilder.java index a466e39bf3f9..d53799fd1102 100644 --- a/src/test/java/seedu/address/testutil/AddressBookBuilder.java +++ b/src/test/java/seedu/address/testutil/AddressBookBuilder.java @@ -2,7 +2,6 @@ import seedu.address.model.AddressBook; import seedu.address.model.person.Person; -import seedu.address.model.person.exceptions.DuplicatePersonException; /** * A utility class to help with building Addressbook objects. @@ -25,11 +24,7 @@ public AddressBookBuilder(AddressBook addressBook) { * Adds a new {@code Person} to the {@code AddressBook} that we are building. */ public AddressBookBuilder withPerson(Person person) { - try { - addressBook.addPerson(person); - } catch (DuplicatePersonException dpe) { - throw new IllegalArgumentException("person is expected to be unique.", dpe); - } + addressBook.addPerson(person); return this; } diff --git a/src/test/java/seedu/address/testutil/TypicalPersons.java b/src/test/java/seedu/address/testutil/TypicalPersons.java index cd5762f2bcd7..fec76fb71293 100644 --- a/src/test/java/seedu/address/testutil/TypicalPersons.java +++ b/src/test/java/seedu/address/testutil/TypicalPersons.java @@ -17,7 +17,6 @@ import seedu.address.model.AddressBook; import seedu.address.model.person.Person; -import seedu.address.model.person.exceptions.DuplicatePersonException; /** * A utility class containing a list of {@code Person} objects to be used in tests. @@ -66,11 +65,7 @@ private TypicalPersons() {} // prevents instantiation public static AddressBook getTypicalAddressBook() { AddressBook ab = new AddressBook(); for (Person person : getTypicalPersons()) { - try { - ab.addPerson(person); - } catch (DuplicatePersonException e) { - throw new AssertionError("not possible", e); - } + ab.addPerson(person); } return ab; } diff --git a/src/test/java/systemtests/AddCommandSystemTest.java b/src/test/java/systemtests/AddCommandSystemTest.java index 4dee13251f3f..a63aefc32bb0 100644 --- a/src/test/java/systemtests/AddCommandSystemTest.java +++ b/src/test/java/systemtests/AddCommandSystemTest.java @@ -42,7 +42,6 @@ import seedu.address.model.person.Name; import seedu.address.model.person.Person; import seedu.address.model.person.Phone; -import seedu.address.model.person.exceptions.DuplicatePersonException; import seedu.address.model.tag.Tag; import seedu.address.testutil.PersonBuilder; import seedu.address.testutil.PersonUtil; @@ -50,7 +49,7 @@ public class AddCommandSystemTest extends AddressBookSystemTest { @Test - public void add() throws Exception { + public void add() { Model model = getModel(); /* ------------------------ Perform add operations on the shown unfiltered list ----------------------------- */ @@ -207,11 +206,7 @@ private void assertCommandSuccess(Person toAdd) { */ private void assertCommandSuccess(String command, Person toAdd) { Model expectedModel = getModel(); - try { - expectedModel.addPerson(toAdd); - } catch (DuplicatePersonException dpe) { - throw new IllegalArgumentException("toAdd already exists in the model.", dpe); - } + expectedModel.addPerson(toAdd); String expectedResultMessage = String.format(AddCommand.MESSAGE_SUCCESS, toAdd); assertCommandSuccess(command, expectedModel, expectedResultMessage); diff --git a/src/test/java/systemtests/EditCommandSystemTest.java b/src/test/java/systemtests/EditCommandSystemTest.java index 38d82c61aaf0..6547b0dbc3df 100644 --- a/src/test/java/systemtests/EditCommandSystemTest.java +++ b/src/test/java/systemtests/EditCommandSystemTest.java @@ -44,7 +44,6 @@ import seedu.address.model.person.Name; import seedu.address.model.person.Person; import seedu.address.model.person.Phone; -import seedu.address.model.person.exceptions.DuplicatePersonException; import seedu.address.model.person.exceptions.PersonNotFoundException; import seedu.address.model.tag.Tag; import seedu.address.testutil.PersonBuilder; @@ -240,9 +239,8 @@ private void assertCommandSuccess(String command, Index toEdit, Person editedPer expectedModel.updatePerson( expectedModel.getFilteredPersonList().get(toEdit.getZeroBased()), editedPerson); expectedModel.updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS); - } catch (DuplicatePersonException | PersonNotFoundException e) { - throw new IllegalArgumentException( - "editedPerson is a duplicate in expectedModel, or it isn't found in the model.", e); + } catch (PersonNotFoundException e) { + throw new IllegalArgumentException("editedPerson isn't found in the model.", e); } assertCommandSuccess(command, expectedModel, From 73693de3a3ada76e08fe72922c15edbaf415e15d Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 7 Aug 2018 23:11:14 +0800 Subject: [PATCH 06/11] model: make PersonNotFoundException a RuntimeException PersonNotFoundException is a checked exception. It is thrown by methods such as `UniquePersonList#remove(Person)`, `UniquePersonList#setPerson(Person, Person)` etc. to signify that certain preconditions were not met -- namely that the person provided to those methods does not exist in the address book/UniquePersonList. Using checked exceptions for such a purpose is slightly useful in that it will force callers to handle the case where the above preconditions are not met, such as when the methods are called with invalid user input. However, it imposes a HUGE cost on callers where the preconditions are already known to be met (e.g. in test code, or when the user input has already been validated before hand). In such a case, callers are forced to add try-catch blocks around the method call even if they know that the exception will never be thrown, bloating up the code. It is also impossible to test the catch blocks as well since correct code will ensure that the precondition holds and thus the exception will never be thrown, leading to reduced code coverage. Checked exceptions also don't work very well with the Java Streams API, since the API doesn't accept lambdas which could throw checked exceptions. In AB-4, the amount of code which benefits from PersonNotFoundException being a checked exception is much smaller than the amount of code which is negatively impacted. As such, let's make the tradeoff in the other direction, by making PersonNotFoundException a RuntimeException. New callers _could_ forget to check that the preconditions hold before calling the methods in question (although test cases should catch that), but this is balanced out by the huge benefit of having more concise and testable code. --- .../seedu/address/logic/commands/DeleteCommand.java | 11 ++--------- .../seedu/address/logic/commands/EditCommand.java | 7 +------ src/main/java/seedu/address/model/AddressBook.java | 10 ++++------ src/main/java/seedu/address/model/Model.java | 13 +++++++------ src/main/java/seedu/address/model/ModelManager.java | 5 ++--- .../address/model/person/UniquePersonList.java | 10 ++++------ .../person/exceptions/PersonNotFoundException.java | 2 +- .../address/logic/commands/CommandTestUtil.java | 9 ++------- .../address/logic/commands/DeleteCommandTest.java | 4 ++-- .../address/logic/commands/EditCommandTest.java | 6 +++--- .../java/systemtests/DeleteCommandSystemTest.java | 7 +------ .../java/systemtests/EditCommandSystemTest.java | 12 +++--------- 12 files changed, 32 insertions(+), 64 deletions(-) diff --git a/src/main/java/seedu/address/logic/commands/DeleteCommand.java b/src/main/java/seedu/address/logic/commands/DeleteCommand.java index 360303a8fc66..d4d3ca82369b 100644 --- a/src/main/java/seedu/address/logic/commands/DeleteCommand.java +++ b/src/main/java/seedu/address/logic/commands/DeleteCommand.java @@ -6,7 +6,6 @@ import seedu.address.commons.core.index.Index; import seedu.address.logic.commands.exceptions.CommandException; import seedu.address.model.person.Person; -import seedu.address.model.person.exceptions.PersonNotFoundException; /** * Deletes a person identified using it's displayed index from the address book. @@ -37,14 +36,8 @@ public CommandResult execute() throws CommandException { } Person personToDelete = lastShownList.get(targetIndex.getZeroBased()); - - try { - model.deletePerson(personToDelete); - model.commitAddressBook(); - } catch (PersonNotFoundException pnfe) { - throw new AssertionError("The target person cannot be missing", pnfe); - } - + model.deletePerson(personToDelete); + model.commitAddressBook(); return new CommandResult(String.format(MESSAGE_DELETE_PERSON_SUCCESS, personToDelete)); } diff --git a/src/main/java/seedu/address/logic/commands/EditCommand.java b/src/main/java/seedu/address/logic/commands/EditCommand.java index 5d79efe7e246..f13f7d1c69e9 100644 --- a/src/main/java/seedu/address/logic/commands/EditCommand.java +++ b/src/main/java/seedu/address/logic/commands/EditCommand.java @@ -23,7 +23,6 @@ import seedu.address.model.person.Name; import seedu.address.model.person.Person; import seedu.address.model.person.Phone; -import seedu.address.model.person.exceptions.PersonNotFoundException; import seedu.address.model.tag.Tag; /** @@ -80,11 +79,7 @@ public CommandResult execute() throws CommandException { throw new CommandException(MESSAGE_DUPLICATE_PERSON); } - try { - model.updatePerson(personToEdit, editedPerson); - } catch (PersonNotFoundException pnfe) { - throw new AssertionError("The target person cannot be missing", pnfe); - } + model.updatePerson(personToEdit, editedPerson); model.updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS); model.commitAddressBook(); return new CommandResult(String.format(MESSAGE_EDIT_PERSON_SUCCESS, editedPerson)); diff --git a/src/main/java/seedu/address/model/AddressBook.java b/src/main/java/seedu/address/model/AddressBook.java index 2c4c06ddbb28..0c9a902a8843 100644 --- a/src/main/java/seedu/address/model/AddressBook.java +++ b/src/main/java/seedu/address/model/AddressBook.java @@ -7,7 +7,6 @@ import javafx.collections.ObservableList; import seedu.address.model.person.Person; import seedu.address.model.person.UniquePersonList; -import seedu.address.model.person.exceptions.PersonNotFoundException; /** * Wraps all data at the address-book level @@ -77,11 +76,10 @@ public void addPerson(Person p) { /** * Replaces the given person {@code target} in the list with {@code editedPerson}. + * {@code target} must exist in the address book. * The person identity of {@code editedPerson} must not be the same as another existing person in the address book. - * - * @throws PersonNotFoundException if {@code target} could not be found in the list. */ - public void updatePerson(Person target, Person editedPerson) throws PersonNotFoundException { + public void updatePerson(Person target, Person editedPerson) { requireNonNull(editedPerson); persons.setPerson(target, editedPerson); @@ -89,9 +87,9 @@ public void updatePerson(Person target, Person editedPerson) throws PersonNotFou /** * Removes {@code key} from this {@code AddressBook}. - * @throws PersonNotFoundException if the {@code key} is not in this {@code AddressBook}. + * {@code key} must exist in the address book. */ - public void removePerson(Person key) throws PersonNotFoundException { + public void removePerson(Person key) { persons.remove(key); } diff --git a/src/main/java/seedu/address/model/Model.java b/src/main/java/seedu/address/model/Model.java index fe6ad8333305..ac4521f33199 100644 --- a/src/main/java/seedu/address/model/Model.java +++ b/src/main/java/seedu/address/model/Model.java @@ -4,7 +4,6 @@ import javafx.collections.ObservableList; import seedu.address.model.person.Person; -import seedu.address.model.person.exceptions.PersonNotFoundException; /** * The API of the Model component. @@ -24,8 +23,11 @@ public interface Model { */ boolean hasPerson(Person person); - /** Deletes the given person. */ - void deletePerson(Person target) throws PersonNotFoundException; + /** + * Deletes the given person. + * The person must exist in the address book. + */ + void deletePerson(Person target); /** * Adds the given person. @@ -35,11 +37,10 @@ public interface Model { /** * Replaces the given person {@code target} with {@code editedPerson}. + * {@code target} must exist in the address book. * The person identity of {@code editedPerson} must not be the same as another existing person in the address book. - * - * @throws PersonNotFoundException if {@code target} could not be found in the list. */ - void updatePerson(Person target, Person editedPerson) throws PersonNotFoundException; + void updatePerson(Person target, Person editedPerson); /** Returns an unmodifiable view of the filtered person list */ ObservableList getFilteredPersonList(); diff --git a/src/main/java/seedu/address/model/ModelManager.java b/src/main/java/seedu/address/model/ModelManager.java index 913cb6864377..b6063a769ff9 100644 --- a/src/main/java/seedu/address/model/ModelManager.java +++ b/src/main/java/seedu/address/model/ModelManager.java @@ -13,7 +13,6 @@ import seedu.address.commons.core.LogsCenter; import seedu.address.commons.events.model.AddressBookChangedEvent; import seedu.address.model.person.Person; -import seedu.address.model.person.exceptions.PersonNotFoundException; /** * Represents the in-memory model of the address book data. @@ -65,7 +64,7 @@ public synchronized boolean hasPerson(Person person) { } @Override - public synchronized void deletePerson(Person target) throws PersonNotFoundException { + public synchronized void deletePerson(Person target) { versionedAddressBook.removePerson(target); indicateAddressBookChanged(); } @@ -78,7 +77,7 @@ public synchronized void addPerson(Person person) { } @Override - public void updatePerson(Person target, Person editedPerson) throws PersonNotFoundException { + public void updatePerson(Person target, Person editedPerson) { requireAllNonNull(target, editedPerson); versionedAddressBook.updatePerson(target, editedPerson); diff --git a/src/main/java/seedu/address/model/person/UniquePersonList.java b/src/main/java/seedu/address/model/person/UniquePersonList.java index 71dc1043651e..65840f3fb0c2 100644 --- a/src/main/java/seedu/address/model/person/UniquePersonList.java +++ b/src/main/java/seedu/address/model/person/UniquePersonList.java @@ -48,11 +48,10 @@ public void add(Person toAdd) { /** * Replaces the person {@code target} in the list with {@code editedPerson}. + * {@code target} must exist in the list. * The person identity of {@code editedPerson} must not be the same as another existing person in the list. - * - * @throws PersonNotFoundException if {@code target} could not be found in the list. */ - public void setPerson(Person target, Person editedPerson) throws PersonNotFoundException { + public void setPerson(Person target, Person editedPerson) { requireNonNull(editedPerson); int index = internalList.indexOf(target); @@ -69,10 +68,9 @@ public void setPerson(Person target, Person editedPerson) throws PersonNotFoundE /** * Removes the equivalent person from the list. - * - * @throws PersonNotFoundException if no such person could be found in the list. + * The person must exist in the list. */ - public void remove(Person toRemove) throws PersonNotFoundException { + public void remove(Person toRemove) { requireNonNull(toRemove); if (!internalList.remove(toRemove)) { throw new PersonNotFoundException(); diff --git a/src/main/java/seedu/address/model/person/exceptions/PersonNotFoundException.java b/src/main/java/seedu/address/model/person/exceptions/PersonNotFoundException.java index f757e25f5566..fa764426ca73 100644 --- a/src/main/java/seedu/address/model/person/exceptions/PersonNotFoundException.java +++ b/src/main/java/seedu/address/model/person/exceptions/PersonNotFoundException.java @@ -3,4 +3,4 @@ /** * Signals that the operation is unable to find the specified person. */ -public class PersonNotFoundException extends Exception {} +public class PersonNotFoundException extends RuntimeException {} diff --git a/src/test/java/seedu/address/logic/commands/CommandTestUtil.java b/src/test/java/seedu/address/logic/commands/CommandTestUtil.java index 32964d022a42..870797d66d83 100644 --- a/src/test/java/seedu/address/logic/commands/CommandTestUtil.java +++ b/src/test/java/seedu/address/logic/commands/CommandTestUtil.java @@ -19,7 +19,6 @@ import seedu.address.model.Model; import seedu.address.model.person.NameContainsKeywordsPredicate; import seedu.address.model.person.Person; -import seedu.address.model.person.exceptions.PersonNotFoundException; import seedu.address.testutil.EditPersonDescriptorBuilder; /** @@ -127,12 +126,8 @@ public static void showPersonAtIndex(Model model, Index targetIndex) { */ public static void deleteFirstPerson(Model model) { Person firstPerson = model.getFilteredPersonList().get(0); - try { - model.deletePerson(firstPerson); - model.commitAddressBook(); - } catch (PersonNotFoundException pnfe) { - throw new AssertionError("Person in filtered list must exist in model.", pnfe); - } + model.deletePerson(firstPerson); + model.commitAddressBook(); } /** diff --git a/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java b/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java index dab1a682a43e..a25f3b30a98b 100644 --- a/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java @@ -31,7 +31,7 @@ public class DeleteCommandTest { private Model model = new ModelManager(getTypicalAddressBook(), new UserPrefs()); @Test - public void execute_validIndexUnfilteredList_success() throws Exception { + public void execute_validIndexUnfilteredList_success() { Person personToDelete = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased()); DeleteCommand deleteCommand = prepareCommand(INDEX_FIRST_PERSON); @@ -53,7 +53,7 @@ public void execute_invalidIndexUnfilteredList_throwsCommandException() { } @Test - public void execute_validIndexFilteredList_success() throws Exception { + public void execute_validIndexFilteredList_success() { showPersonAtIndex(model, INDEX_FIRST_PERSON); Person personToDelete = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased()); diff --git a/src/test/java/seedu/address/logic/commands/EditCommandTest.java b/src/test/java/seedu/address/logic/commands/EditCommandTest.java index 06612527d891..27c2947b591a 100644 --- a/src/test/java/seedu/address/logic/commands/EditCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/EditCommandTest.java @@ -39,7 +39,7 @@ public class EditCommandTest { private Model model = new ModelManager(getTypicalAddressBook(), new UserPrefs()); @Test - public void execute_allFieldsSpecifiedUnfilteredList_success() throws Exception { + public void execute_allFieldsSpecifiedUnfilteredList_success() { Person editedPerson = new PersonBuilder().build(); EditPersonDescriptor descriptor = new EditPersonDescriptorBuilder(editedPerson).build(); EditCommand editCommand = prepareCommand(INDEX_FIRST_PERSON, descriptor); @@ -54,7 +54,7 @@ public void execute_allFieldsSpecifiedUnfilteredList_success() throws Exception } @Test - public void execute_someFieldsSpecifiedUnfilteredList_success() throws Exception { + public void execute_someFieldsSpecifiedUnfilteredList_success() { Index indexLastPerson = Index.fromOneBased(model.getFilteredPersonList().size()); Person lastPerson = model.getFilteredPersonList().get(indexLastPerson.getZeroBased()); @@ -89,7 +89,7 @@ public void execute_noFieldSpecifiedUnfilteredList_success() { } @Test - public void execute_filteredList_success() throws Exception { + public void execute_filteredList_success() { showPersonAtIndex(model, INDEX_FIRST_PERSON); Person personInFilteredList = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased()); diff --git a/src/test/java/systemtests/DeleteCommandSystemTest.java b/src/test/java/systemtests/DeleteCommandSystemTest.java index 97d42add1209..29fbb5fc3930 100644 --- a/src/test/java/systemtests/DeleteCommandSystemTest.java +++ b/src/test/java/systemtests/DeleteCommandSystemTest.java @@ -19,7 +19,6 @@ import seedu.address.logic.commands.UndoCommand; import seedu.address.model.Model; import seedu.address.model.person.Person; -import seedu.address.model.person.exceptions.PersonNotFoundException; public class DeleteCommandSystemTest extends AddressBookSystemTest { @@ -118,11 +117,7 @@ public void delete() { */ private Person removePerson(Model model, Index index) { Person targetPerson = getPerson(model, index); - try { - model.deletePerson(targetPerson); - } catch (PersonNotFoundException pnfe) { - throw new AssertionError("targetPerson is retrieved from model.", pnfe); - } + model.deletePerson(targetPerson); return targetPerson; } diff --git a/src/test/java/systemtests/EditCommandSystemTest.java b/src/test/java/systemtests/EditCommandSystemTest.java index 6547b0dbc3df..9e9e4572ddb2 100644 --- a/src/test/java/systemtests/EditCommandSystemTest.java +++ b/src/test/java/systemtests/EditCommandSystemTest.java @@ -44,7 +44,6 @@ import seedu.address.model.person.Name; import seedu.address.model.person.Person; import seedu.address.model.person.Phone; -import seedu.address.model.person.exceptions.PersonNotFoundException; import seedu.address.model.tag.Tag; import seedu.address.testutil.PersonBuilder; import seedu.address.testutil.PersonUtil; @@ -52,7 +51,7 @@ public class EditCommandSystemTest extends AddressBookSystemTest { @Test - public void edit() throws Exception { + public void edit() { Model model = getModel(); /* ----------------- Performing edit operation while an unfiltered list is being shown ---------------------- */ @@ -235,13 +234,8 @@ private void assertCommandSuccess(String command, Index toEdit, Person editedPer private void assertCommandSuccess(String command, Index toEdit, Person editedPerson, Index expectedSelectedCardIndex) { Model expectedModel = getModel(); - try { - expectedModel.updatePerson( - expectedModel.getFilteredPersonList().get(toEdit.getZeroBased()), editedPerson); - expectedModel.updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS); - } catch (PersonNotFoundException e) { - throw new IllegalArgumentException("editedPerson isn't found in the model.", e); - } + expectedModel.updatePerson(expectedModel.getFilteredPersonList().get(toEdit.getZeroBased()), editedPerson); + expectedModel.updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS); assertCommandSuccess(command, expectedModel, String.format(EditCommand.MESSAGE_EDIT_PERSON_SUCCESS, editedPerson), expectedSelectedCardIndex); From 1c39319fe333c53037f0f03d37b8693a41e905fb Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 7 Aug 2018 23:14:34 +0800 Subject: [PATCH 07/11] AddressBookSystemTest: remove unnecessary catch block The code in the `try` block does not throw any checked exceptions. --- .../systemtests/AddressBookSystemTest.java | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/test/java/systemtests/AddressBookSystemTest.java b/src/test/java/systemtests/AddressBookSystemTest.java index 53e13959d136..bf1743e3d16e 100644 --- a/src/test/java/systemtests/AddressBookSystemTest.java +++ b/src/test/java/systemtests/AddressBookSystemTest.java @@ -274,17 +274,13 @@ protected void assertStatusBarUnchangedExceptSyncStatus() { * Asserts that the starting state of the application is correct. */ private void assertApplicationStartingStateIsCorrect() { - try { - assertEquals("", getCommandBox().getInput()); - assertEquals("", getResultDisplay().getText()); - assertListMatching(getPersonListPanel(), getModel().getFilteredPersonList()); - assertEquals(MainApp.class.getResource(FXML_FILE_FOLDER + DEFAULT_PAGE), getBrowserPanel().getLoadedUrl()); - assertEquals(Paths.get(".").resolve(testApp.getStorageSaveLocation()).toString(), - getStatusBarFooter().getSaveLocation()); - assertEquals(SYNC_STATUS_INITIAL, getStatusBarFooter().getSyncStatus()); - } catch (Exception e) { - throw new AssertionError("Starting state is wrong.", e); - } + assertEquals("", getCommandBox().getInput()); + assertEquals("", getResultDisplay().getText()); + assertListMatching(getPersonListPanel(), getModel().getFilteredPersonList()); + assertEquals(MainApp.class.getResource(FXML_FILE_FOLDER + DEFAULT_PAGE), getBrowserPanel().getLoadedUrl()); + assertEquals(Paths.get(".").resolve(testApp.getStorageSaveLocation()).toString(), + getStatusBarFooter().getSaveLocation()); + assertEquals(SYNC_STATUS_INITIAL, getStatusBarFooter().getSyncStatus()); } /** From bc1b0686b21ab8ad3a511b154bd634ee056daef1 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 7 Aug 2018 23:26:28 +0800 Subject: [PATCH 08/11] commons: remove DuplicateDataException It is not used anymore. --- .../commons/exceptions/DuplicateDataException.java | 10 ---------- 1 file changed, 10 deletions(-) delete mode 100644 src/main/java/seedu/address/commons/exceptions/DuplicateDataException.java diff --git a/src/main/java/seedu/address/commons/exceptions/DuplicateDataException.java b/src/main/java/seedu/address/commons/exceptions/DuplicateDataException.java deleted file mode 100644 index 17aa63d5020c..000000000000 --- a/src/main/java/seedu/address/commons/exceptions/DuplicateDataException.java +++ /dev/null @@ -1,10 +0,0 @@ -package seedu.address.commons.exceptions; - -/** - * Signals an error caused by duplicate data where there should be none. - */ -public abstract class DuplicateDataException extends IllegalValueException { - public DuplicateDataException(String message) { - super(message); - } -} From e7473900c12426764aebbc582f2ecebb247a6b89 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Wed, 8 Aug 2018 16:29:36 +0800 Subject: [PATCH 09/11] UniquePersonList: check for null arguments more thoroughly Some methods do not explicitly check that their arguments are non-null with `requireNonNull(...)`. Let's fix them to be consistent with the rest of the code base. --- src/main/java/seedu/address/model/person/UniquePersonList.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/seedu/address/model/person/UniquePersonList.java b/src/main/java/seedu/address/model/person/UniquePersonList.java index 65840f3fb0c2..c82d40dd628f 100644 --- a/src/main/java/seedu/address/model/person/UniquePersonList.java +++ b/src/main/java/seedu/address/model/person/UniquePersonList.java @@ -52,7 +52,7 @@ public void add(Person toAdd) { * The person identity of {@code editedPerson} must not be the same as another existing person in the list. */ public void setPerson(Person target, Person editedPerson) { - requireNonNull(editedPerson); + requireAllNonNull(target, editedPerson); int index = internalList.indexOf(target); if (index == -1) { @@ -78,6 +78,7 @@ public void remove(Person toRemove) { } public void setPersons(UniquePersonList replacement) { + requireNonNull(replacement); this.internalList.setAll(replacement.internalList); } From 8e370e418958244d8a842b32161762d545f401e7 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Thu, 9 Aug 2018 14:23:49 +0800 Subject: [PATCH 10/11] UniquePersonListTest: move to `seedu.address.model.person` package Since UniquePersonList is in the `seedu.address.model.person` package, its corresponding test UniquePersonListTest should be in the same package as well. --- .../address/model/{ => person}/UniquePersonListTest.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) rename src/test/java/seedu/address/model/{ => person}/UniquePersonListTest.java (86%) diff --git a/src/test/java/seedu/address/model/UniquePersonListTest.java b/src/test/java/seedu/address/model/person/UniquePersonListTest.java similarity index 86% rename from src/test/java/seedu/address/model/UniquePersonListTest.java rename to src/test/java/seedu/address/model/person/UniquePersonListTest.java index f92bb836c1ae..71885bd430d2 100644 --- a/src/test/java/seedu/address/model/UniquePersonListTest.java +++ b/src/test/java/seedu/address/model/person/UniquePersonListTest.java @@ -1,11 +1,9 @@ -package seedu.address.model; +package seedu.address.model.person; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; -import seedu.address.model.person.UniquePersonList; - public class UniquePersonListTest { @Rule public ExpectedException thrown = ExpectedException.none(); From b2875e000e2b97ca77402bd65e6269a1e6994b2e Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Wed, 8 Aug 2018 16:19:10 +0800 Subject: [PATCH 11/11] UniquePersonListTest: add some more tests More test coverage is always nice. In particular, we want to check that UniquePersonList's methods throw DuplicatePersonException and PersonNotFoundException where relevant. These cases were not tested by existing test code. --- .../model/person/UniquePersonListTest.java | 170 +++++++++++++++++- 1 file changed, 169 insertions(+), 1 deletion(-) diff --git a/src/test/java/seedu/address/model/person/UniquePersonListTest.java b/src/test/java/seedu/address/model/person/UniquePersonListTest.java index 71885bd430d2..6389cd81703f 100644 --- a/src/test/java/seedu/address/model/person/UniquePersonListTest.java +++ b/src/test/java/seedu/address/model/person/UniquePersonListTest.java @@ -1,16 +1,184 @@ package seedu.address.model.person; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static seedu.address.logic.commands.CommandTestUtil.VALID_ADDRESS_BOB; +import static seedu.address.logic.commands.CommandTestUtil.VALID_TAG_HUSBAND; +import static seedu.address.testutil.TypicalPersons.ALICE; +import static seedu.address.testutil.TypicalPersons.BOB; + +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; +import seedu.address.model.person.exceptions.DuplicatePersonException; +import seedu.address.model.person.exceptions.PersonNotFoundException; +import seedu.address.testutil.PersonBuilder; + public class UniquePersonListTest { @Rule public ExpectedException thrown = ExpectedException.none(); + private final UniquePersonList uniquePersonList = new UniquePersonList(); + + @Test + public void contains_nullPerson_throwsNullPointerException() { + thrown.expect(NullPointerException.class); + uniquePersonList.contains(null); + } + + @Test + public void contains_personNotInList_returnsFalse() { + assertFalse(uniquePersonList.contains(ALICE)); + } + + @Test + public void contains_personInList_returnsTrue() { + uniquePersonList.add(ALICE); + assertTrue(uniquePersonList.contains(ALICE)); + } + + @Test + public void contains_personWithSameIdentityFieldsInList_returnsTrue() { + uniquePersonList.add(ALICE); + Person editedAlice = new PersonBuilder(ALICE).withAddress(VALID_ADDRESS_BOB).withTags(VALID_TAG_HUSBAND) + .build(); + assertTrue(uniquePersonList.contains(editedAlice)); + } + + @Test + public void add_nullPerson_throwsNullPointerException() { + thrown.expect(NullPointerException.class); + uniquePersonList.add(null); + } + + @Test + public void add_duplicatePerson_throwsDuplicatePersonException() { + uniquePersonList.add(ALICE); + thrown.expect(DuplicatePersonException.class); + uniquePersonList.add(ALICE); + } + + @Test + public void setPerson_nullTargetPerson_throwsNullPointerException() { + thrown.expect(NullPointerException.class); + uniquePersonList.setPerson(null, ALICE); + } + + @Test + public void setPerson_nullEditedPerson_throwsNullPointerException() { + thrown.expect(NullPointerException.class); + uniquePersonList.setPerson(ALICE, null); + } + + @Test + public void setPerson_targetPersonNotInList_throwsPersonNotFoundException() { + thrown.expect(PersonNotFoundException.class); + uniquePersonList.setPerson(ALICE, ALICE); + } + + @Test + public void setPerson_editedPersonIsSamePerson_success() { + uniquePersonList.add(ALICE); + uniquePersonList.setPerson(ALICE, ALICE); + UniquePersonList expectedUniquePersonList = new UniquePersonList(); + expectedUniquePersonList.add(ALICE); + assertEquals(expectedUniquePersonList, uniquePersonList); + } + + @Test + public void setPerson_editedPersonHasSameIdentity_success() { + uniquePersonList.add(ALICE); + Person editedAlice = new PersonBuilder(ALICE).withAddress(VALID_ADDRESS_BOB).withTags(VALID_TAG_HUSBAND) + .build(); + uniquePersonList.setPerson(ALICE, editedAlice); + UniquePersonList expectedUniquePersonList = new UniquePersonList(); + expectedUniquePersonList.add(editedAlice); + assertEquals(expectedUniquePersonList, uniquePersonList); + } + + @Test + public void setPerson_editedPersonHasDifferentIdentity_success() { + uniquePersonList.add(ALICE); + uniquePersonList.setPerson(ALICE, BOB); + UniquePersonList expectedUniquePersonList = new UniquePersonList(); + expectedUniquePersonList.add(BOB); + assertEquals(expectedUniquePersonList, uniquePersonList); + } + + @Test + public void setPerson_editedPersonHasNonUniqueIdentity_throwsDuplicatePersonException() { + uniquePersonList.add(ALICE); + uniquePersonList.add(BOB); + thrown.expect(DuplicatePersonException.class); + uniquePersonList.setPerson(ALICE, BOB); + } + + @Test + public void remove_nullPerson_throwsNullPointerException() { + thrown.expect(NullPointerException.class); + uniquePersonList.remove(null); + } + + @Test + public void remove_personDoesNotExist_throwsPersonNotFoundException() { + thrown.expect(PersonNotFoundException.class); + uniquePersonList.remove(ALICE); + } + + @Test + public void remove_existingPerson_removesPerson() { + uniquePersonList.add(ALICE); + uniquePersonList.remove(ALICE); + UniquePersonList expectedUniquePersonList = new UniquePersonList(); + assertEquals(expectedUniquePersonList, uniquePersonList); + } + + @Test + public void setPersons_nullUniquePersonList_throwsNullPointerException() { + thrown.expect(NullPointerException.class); + uniquePersonList.setPersons((UniquePersonList) null); + } + + @Test + public void setPersons_uniquePersonList_replacesOwnListWithProvidedUniquePersonList() { + uniquePersonList.add(ALICE); + UniquePersonList expectedUniquePersonList = new UniquePersonList(); + expectedUniquePersonList.add(BOB); + uniquePersonList.setPersons(expectedUniquePersonList); + assertEquals(expectedUniquePersonList, uniquePersonList); + } + + @Test + public void setPersons_nullList_throwsNullPointerException() { + thrown.expect(NullPointerException.class); + uniquePersonList.setPersons((List) null); + } + + @Test + public void setPersons_list_replacesOwnListWithProvidedList() { + uniquePersonList.add(ALICE); + List personList = Collections.singletonList(BOB); + uniquePersonList.setPersons(personList); + UniquePersonList expectedUniquePersonList = new UniquePersonList(); + expectedUniquePersonList.add(BOB); + assertEquals(expectedUniquePersonList, uniquePersonList); + } + + @Test + public void setPersons_listWithDuplicatePersons_throwsDuplicatePersonException() { + List listWithDuplicatePersons = Arrays.asList(ALICE, ALICE); + thrown.expect(DuplicatePersonException.class); + uniquePersonList.setPersons(listWithDuplicatePersons); + } + @Test public void asUnmodifiableObservableList_modifyList_throwsUnsupportedOperationException() { - UniquePersonList uniquePersonList = new UniquePersonList(); thrown.expect(UnsupportedOperationException.class); uniquePersonList.asUnmodifiableObservableList().remove(0); }