model: convert checked exceptions to runtime exceptions #896

pyokagan · 2018-08-07T15:15:50Z

DuplicatePersonException and PersonNotFoundException are checked
exceptions. They are thrown by methods to signify that their relevant
preconditions were 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
and PersonNotFoundException being checked exceptions 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 and PersonNotFoundException runtime exceptions.
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.

CanIHasReview-bot · 2018-08-07T15:15:51Z

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

CanIHasReview-bot · 2018-08-08T10:35:09Z

v1

@pyokagan submitted v1 for review.

[v1 1/10] Remove redundant throws clauses
[v1 2/10] model: expose hasPerson(Person) functionality
[v1 3/10] AddCommand: explicitly perform duplicate person check
[v1 4/10] EditCommand: explicitly perform duplicate person check
[v1 5/10] model: make DuplicatePersonException a RuntimeException
[v1 6/10] model: make PersonNotFoundException a RuntimeException
[v1 7/10] AddressBookSystemTest: remove unnecessary catch block
[v1 8/10] commons: remove DuplicateDataException
[v1 9/10] UniquePersonList: check for null arguments more thoroughly
[v1 10/10] UniquePersonListTest: add some more tests

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/896/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

eugenepeh

[10/10] Since UniquePersonList is in the package seedu.address.model.person;, UniquePersonListTest should be correspondingly the same.

yamidark · 2018-08-08T14:04:10Z

src/test/java/seedu/address/logic/commands/AddCommandTest.java

@@ -110,13 +110,12 @@ public ReadOnlyAddressBook getAddressBook() {
        }

        @Override
-        public void deletePerson(Person target) throws PersonNotFoundException {


[1/10] PersonNotFoundException import not removed

Sorry, I'll remember to run tests on every commit next time ;_;

yamidark

[5/10]
nit in last paragraph:
As such, let's ~~making~~ make the tradeoff in the other direction, by making
DuplicatePersonException a RuntimeException. New callers could forget to...

yamidark

[6/10]
same as [5/10]

Zhiyuan-Amos

[3/10]

Zhiyuan-Amos · 2018-08-08T14:24:27Z

src/test/java/seedu/address/logic/commands/AddCommandTest.java

@@ -181,6 +182,11 @@ public ReadOnlyAddressBook getAddressBook() {
    private class ModelStubAcceptingPersonAdded extends ModelStub {
        final ArrayList<Person> personsAdded = new ArrayList<>();

+        @Override
+        public boolean hasPerson(Person person) {
+            return personsAdded.stream().anyMatch(addedPerson -> addedPerson.isSamePerson(person));


lambda expression can be simplified to : person::isSamePerson

Zhiyuan-Amos

[10/10] Since we are on Java 9, should we use List.of(...) instead of Arrays.asList(...)?

Zhiyuan-Amos · 2018-08-08T14:34:25Z

src/test/java/seedu/address/model/UniquePersonListTest.java

+    @Test
+    public void setPerson_editedPersonIsSamePerson_success() {
+        uniquePersonList.add(ALICE);
+        uniquePersonList.setPerson(ALICE, ALICE);


Should we do uniquePersonList.setPerson(ALICE, new PersonBuilder(ALICE).build()); instead?
It's unclear here why we are using the same object references.

If they refer to the same object (for whatever reason), the API should still allow it, right?

Yeah, after some more thought, I think I'll keep it. The "different object reference case" is already tested in setPerson_editedPersonHasSameIdentity_success

pyokagan · 2018-08-08T14:47:58Z

@yamidark Sorry, please do concentrate on se-edu/addressbook-level2#388 , I think @damithc would prefer that. Sorry for requesting your review and taking your time, this PR is not so important.

Zhiyuan-Amos

[5/10]

Zhiyuan-Amos · 2018-08-08T14:51:20Z

src/main/java/seedu/address/model/util/SampleDataUtil.java

-            throw new AssertionError("sample data cannot contain duplicate persons", e);
+        AddressBook sampleAb = new AddressBook();
+        for (Person samplePerson : getSamplePersons()) {
+            sampleAb.addPerson(samplePerson);


will this PR be simplifying some of the loops such as this one?
Arrays.stream(getSamplePersons()).forEach(sampleAb::addPerson);

Since you mentioned in your commit message that:

Checked exceptions also don't work very well with the Java Streams API, since the API doesn't accept lambas which could throw checked exceptions.

Eh let's keep that for another PR, to avoid delaying this one.

Ok! Oh I spotted a spelling mistake for your commit msg: lambas :p

pyokagan · 2018-08-09T02:13:25Z

@eugenepeh

[10/10] Since UniquePersonList is in the package seedu.address.model.person;, UniquePersonListTest should be correspondingly the same.

fyi, this is not a new file, but I'll do it anyway in this PR since it's easy enough to do.

These redundant throws clauses were identified using IntelliJ's Analyze->Inspect Code feature, and analyzing the whole project.

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.

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.

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.

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.

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.

The code in the `try` block does not throw any checked exceptions.

It is not used anymore.

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.

Since UniquePersonList is in the `seedu.address.model.person` package, its corresponding test UniquePersonListTest should be in the same package as well.

pyokagan · 2018-08-09T06:27:13Z

@Zhiyuan-Amos

Since we are on Java 9, should we use List.of(...) instead of Arrays.asList(...)?

I considered that, but we don't use List.of(...) at all in our code base, and this PR may not be a good time to "break new ground" in that sense. Putting aside immutability, Arrays.asList(...) is the same as List.of(...) and not that many more characters to type.

I'll wait for #871

CanIHasReview-bot · 2018-08-09T06:51:57Z

v2

@pyokagan submitted v2 for review.

[v2 1/11] Remove redundant throws clauses
[v2 2/11] model: expose hasPerson(Person) functionality
[v2 3/11] AddCommand: explicitly perform duplicate person check
[v2 4/11] EditCommand: explicitly perform duplicate person check
[v2 5/11] model: make DuplicatePersonException a RuntimeException
[v2 6/11] model: make PersonNotFoundException a RuntimeException
[v2 7/11] AddressBookSystemTest: remove unnecessary catch block
[v2 8/11] commons: remove DuplicateDataException
[v2 9/11] UniquePersonList: check for null arguments more thoroughly
[v2 10/11] UniquePersonListTest: move to `seedu.address.model.person` package
[v2 11/11] UniquePersonListTest: add some more tests

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/896/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

pyokagan · 2018-08-09T07:00:13Z

[3/11] Also added some requireNonNull()s, missed that out in v1.

Zhiyuan-Amos

Just a minor nit 👍

Zhiyuan-Amos · 2018-08-09T07:55:49Z

src/test/java/seedu/address/model/person/UniquePersonListTest.java

+    @Test
+    public void setPersons_list_replacesOwnListWithProvidedList() {
+        uniquePersonList.add(ALICE);
+        List<Person> personList = Arrays.asList(BOB);


IntelliJ would prefer you to use Collections.singletonList() here. :p

Foiled again.

Does it still prefer Collections.singletonList() over List.of(), I wonder.

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.

CanIHasReview-bot · 2018-08-09T09:44:24Z

v3

@pyokagan submitted v3 for review.

[v3 1/11] Remove redundant throws clauses
[v3 2/11] model: expose hasPerson(Person) functionality
[v3 3/11] AddCommand: explicitly perform duplicate person check
[v3 4/11] EditCommand: explicitly perform duplicate person check
[v3 5/11] model: make DuplicatePersonException a RuntimeException
[v3 6/11] model: make PersonNotFoundException a RuntimeException
[v3 7/11] AddressBookSystemTest: remove unnecessary catch block
[v3 8/11] commons: remove DuplicateDataException
[v3 9/11] UniquePersonList: check for null arguments more thoroughly
[v3 10/11] UniquePersonListTest: move to `seedu.address.model.person` package
[v3 11/11] UniquePersonListTest: add some more tests

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/896/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

pyokagan force-pushed the no-checked-exceptions branch 3 times, most recently from 42def33 to c2e22a1 Compare August 8, 2018 08:33

se-edu deleted a comment Aug 8, 2018

pyokagan force-pushed the no-checked-exceptions branch 3 times, most recently from 5939b6c to 1185ff5 Compare August 8, 2018 10:27

CanIHasReview-bot added the s.ToReview label Aug 8, 2018

pyokagan requested a review from a team August 8, 2018 10:35

eugenepeh suggested changes Aug 8, 2018

View reviewed changes

pyokagan requested review from vivekscl, yamidark and Zhiyuan-Amos August 8, 2018 13:24

yamidark reviewed Aug 8, 2018

View reviewed changes

Zhiyuan-Amos reviewed Aug 8, 2018

View reviewed changes

pyokagan added 7 commits August 9, 2018 14:18

Remove redundant throws clauses

000dbd1

These redundant throws clauses were identified using IntelliJ's Analyze->Inspect Code feature, and analyzing the whole project.

AddressBookSystemTest: remove unnecessary catch block

1c39319

The code in the `try` block does not throw any checked exceptions.

pyokagan added 3 commits August 9, 2018 14:22

commons: remove DuplicateDataException

bc1b068

It is not used anymore.

UniquePersonList: check for null arguments more thoroughly

e747390

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.

UniquePersonListTest: move to seedu.address.model.person package

8e370e4

Since UniquePersonList is in the `seedu.address.model.person` package, its corresponding test UniquePersonListTest should be in the same package as well.

pyokagan force-pushed the no-checked-exceptions branch from 1185ff5 to 560a61a Compare August 9, 2018 06:30

pyokagan added s.Ongoing and removed s.ToReview labels Aug 9, 2018

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Aug 9, 2018

CanIHasReview-bot requested a review from eugenepeh August 9, 2018 06:52

pyokagan requested a review from Zhiyuan-Amos August 9, 2018 06:53

Zhiyuan-Amos approved these changes Aug 9, 2018

View reviewed changes

Zhiyuan-Amos mentioned this pull request Aug 9, 2018

Model: Add deleteTag(Tag) #790

Closed

pyokagan added s.Ongoing and removed s.ToReview labels Aug 9, 2018

UniquePersonListTest: add some more tests

b2875e0

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.

pyokagan force-pushed the no-checked-exceptions branch from 560a61a to b2875e0 Compare August 9, 2018 09:18

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Aug 9, 2018

pyokagan added s.ToMerge and removed s.ToReview labels Aug 9, 2018

pyokagan requested a review from damithc August 9, 2018 09:45

damithc approved these changes Aug 9, 2018

View reviewed changes

eugenepeh approved these changes Aug 9, 2018

View reviewed changes

pyokagan removed the request for review from vivekscl August 9, 2018 09:57

pyokagan merged commit db126b9 into se-edu:master Aug 9, 2018

pyokagan deleted the no-checked-exceptions branch December 4, 2018 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

model: convert checked exceptions to runtime exceptions #896

model: convert checked exceptions to runtime exceptions #896

pyokagan commented Aug 7, 2018 •

edited

Loading

CanIHasReview-bot commented Aug 7, 2018

CanIHasReview-bot commented Aug 8, 2018

eugenepeh left a comment

yamidark Aug 8, 2018

pyokagan Aug 8, 2018

yamidark left a comment •

edited

Loading

yamidark left a comment

Zhiyuan-Amos left a comment

Zhiyuan-Amos Aug 8, 2018

Zhiyuan-Amos left a comment

Zhiyuan-Amos Aug 8, 2018

pyokagan Aug 9, 2018

pyokagan Aug 9, 2018

pyokagan commented Aug 8, 2018

Zhiyuan-Amos left a comment

Zhiyuan-Amos Aug 8, 2018

pyokagan Aug 9, 2018

Zhiyuan-Amos Aug 9, 2018

pyokagan commented Aug 9, 2018

pyokagan commented Aug 9, 2018

CanIHasReview-bot commented Aug 9, 2018

pyokagan commented Aug 9, 2018

Zhiyuan-Amos left a comment

Zhiyuan-Amos Aug 9, 2018

pyokagan Aug 9, 2018

CanIHasReview-bot commented Aug 9, 2018

model: convert checked exceptions to runtime exceptions #896

model: convert checked exceptions to runtime exceptions #896

Conversation

pyokagan commented Aug 7, 2018 • edited Loading

CanIHasReview-bot commented Aug 7, 2018

CanIHasReview-bot commented Aug 8, 2018

v1

eugenepeh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamidark left a comment • edited Loading

Choose a reason for hiding this comment

yamidark left a comment

Choose a reason for hiding this comment

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyokagan commented Aug 8, 2018

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyokagan commented Aug 9, 2018

pyokagan commented Aug 9, 2018

CanIHasReview-bot commented Aug 9, 2018

v2

pyokagan commented Aug 9, 2018

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Aug 9, 2018

v3

pyokagan commented Aug 7, 2018 •

edited

Loading

yamidark left a comment •

edited

Loading