model: convert checked exceptions to runtime exceptions #896

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.

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.

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

Commits on Aug 9, 2018