Reduce unnecessary asserts #239 #414

Zhiyuan-Amos · 2017-05-15T08:50:15Z

Fixes #239

Proposed merge commit message:

Parameter validation is done by using asserts, and throws AssertionError
when validation fails.

Asserts may not always be enabled. Furthermore, specific exceptions
such as NullPointerException should be thrown to make the error more
explicit.

Let's
    - Replace the asserts for public APIs with checkNotNull() and
      checkArgument(), which throws an appropriate exception when 
      validation fails.
    - Enhance CollectionUtil#isAnyNull(…) to do the same operation as
      Objects#requireNonNull(T), and also able to take in multiple 
      parameters.

CanIHasReview-bot · 2017-05-16T03:34:33Z

v1

@Zhiyuan-Amos submitted v1 for review.

[v1 1/2] Remove unnecessary asserts
[v1 2/2] Refactor asserts used for parameter validation

(📚 Archive)

Checkout this PR version locally

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

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

yamgent

[v1]

~~EditCommand#createEditedPerson() and EditCommandParser#parseTagsForEdit() still uses assert keyword.~~

yamgent

[v1 1/2]
Just a clarification question.

yamgent · 2017-05-16T05:20:41Z

src/main/java/seedu/address/model/person/UniquePersonList.java

@@ -25,7 +25,6 @@
     * Returns true if the list contains an equivalent person as the given argument.
     */
    public boolean contains(ReadOnlyPerson toCheck) {
-        assert toCheck != null;


Does ObservableList allow null elements? If it does, then internalLIst.contains() will not trigger an NPE.

you are right, it allows null elements. If the list allows for null elements, does it mean that we should allow the method to be called contains(null)? Or should we require it to be non-null?

For our side, it should prohibit accepting null, so we should use checkNotNull()

yamgent

[v1 2/2]

Replace asserts with proper exceptions for parameter validation

Parameter validation is done by using asserts, and throws AssertionError
when validation fails.

Asserts may not always be enabled. Furthermore, specific exceptions
such as NullPointerException should be thrown to make the error more
explicit.

Let's replace the asserts with checkNotNull() and checkArgument(),
which throws an appropriate exception when validation fails.

CanIHasReview-bot · 2017-05-16T07:52:30Z

v2

@Zhiyuan-Amos submitted v2 for review.

[v2 1/1] Replace asserts with proper exceptions for parameter validation

(📚 Archive) (📈 Interdiff between v1 and v2)

Checkout this PR version locally

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

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

yamgent · 2017-05-17T01:11:03Z

@pyokagan @MightyCupcakes For your review please, as both of you have previously suggested a guideline on how we can replace the asserts. :)

pyokagan · 2017-05-18T05:16:42Z

Check if any of the comments from #300 apply here.

pyokagan · 2017-05-18T05:22:47Z

It may be a good idea to start off from the commits in #300, rather than starting from scratch (so reviewers don't have to make the same comments).

Zhiyuan-Amos · 2017-05-18T06:06:49Z

Yup I've referred to the commits from #300. We discussed with prof, prof's opinion is that we should replace assert with checkNotNull for every public method, even though there may be methods inside the method that will throw NPE.

Rationale is: Some of the method calls that throw NPE are found in the middle of the execution of the method. The code executed earlier in the method may have persistent effects, which may result in unintended changes to the program when the app crashes due to NPE.

pyokagan · 2017-05-18T08:53:44Z

@Zhiyuan-Amos

we should replace assert with checkNotNull for every public method, even though there may be methods inside the method that will throw NPE.

OK. That should have been in the commit message.

How about other differences between this PR and #300? E.g., the use of Guava's checkNotNull() instead of JDK's requireNotNull()? As well as the other stylistic differences like the use of the:

this.someField = requireNonNull(someField);

pattern?

pyokagan · 2017-05-18T08:55:55Z

src/main/java/seedu/address/model/ModelManager.java

@@ -30,7 +33,7 @@
     */
    public ModelManager(ReadOnlyAddressBook addressBook, UserPrefs userPrefs) {
        super();
-        assert !CollectionUtil.isAnyNull(addressBook, userPrefs);
+        checkArgument(!CollectionUtil.isAnyNull(addressBook, userPrefs));


Should this be throwing an NullPointerException or an IllegalArgumentException?

Probably NullPointerException, I think, since this is implicitly just checking the arguments to this constructor.

Yup this is a good catch. Perhaps CollectionUtil.isAnyNull should throw NullPointerException if any of the variables are null?

Currently CollectionUtil.isAnyNull is only used in assert statements so when assert is enabled, they throw AssertionError. As such, if we let CollectionUtil.isAnyNull throw NPE instead, I don't think there's much that need to be changed in the codebase :>

Edit: We can rename it as well to be requireNonNull to sound similar to Objects.requireNonNull

~~@Zhiyuan-Amos I think this still doesn't throw NullPointerException?~~

pyokagan · 2017-05-18T09:03:22Z

the use of Guava's checkNotNull() instead of JDK's requireNotNull()?

I think that it's not good to use Guava's functions actually, as checking arguments and not-null has to be done throughout the code-base and it would mean that the code in addressbook-level4 can't be trivially shared with the other levels (since they are not supposed to have access to a java build system like gradle). Instead, I think it's better to just use the standard library's requireNotNull() and write our own checkArgument().

CanIHasReview-bot · 2017-05-19T05:33:47Z

v3

@Zhiyuan-Amos submitted v3 for review.

(📚 Archive) (📈 Interdiff between v2 and v3)

Checkout this PR version locally

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

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

limmlingg · 2017-05-19T05:41:30Z

src/main/java/seedu/address/commons/util/AppUtil.java

+     */
+    public static void checkArgument(Boolean condition, String errorMessage) {
+        if (!condition) {
+            throw new IllegalArgumentException();


errorMessage isn't used.

CanIHasReview-bot · 2017-05-29T07:27:09Z

v7

@Zhiyuan-Amos submitted v7 for review.

(📚 Archive) (📈 Interdiff between v6 and v7)

Checkout this PR version locally

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

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

MightyCupcakes

Commit message:

Let's replace the asserts with checkNotNull() and checkArgument(),
which throws an appropriate exception when validation fails.

I thought we are using requireNonNull?

MightyCupcakes

Commit message:

Java native Objects#requireNonNull(T) can only take in ...

Java -> Java's

CanIHasReview-bot · 2017-05-30T05:05:46Z

v8

@Zhiyuan-Amos submitted v8 for review.

(📚 Archive) (📈 Interdiff between v7 and v8)

Checkout this PR version locally

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

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

Parameter validation is done by using asserts, and throws AssertionError when validation fails. Asserts may not always be enabled. Furthermore, specific exceptions such as NullPointerException should be thrown to make the error more explicit. Let's replace the asserts with requireNonNull() and checkArgument(), which throws an appropriate exception when validation fails.

CanIHasReview-bot · 2017-05-30T07:42:35Z

v9

@Zhiyuan-Amos submitted v9 for review.

(📚 Archive) (📈 Interdiff between v8 and v9)

Checkout this PR version locally

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

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

Zhiyuan-Amos · 2017-05-30T07:43:35Z

added tests for AppUtil#checkArgument

pyokagan · 2017-05-30T07:47:22Z

src/main/java/seedu/address/commons/util/CollectionUtil.java

     */
-    public static boolean isAnyNull(Collection<?> items) {
-        return items.stream().anyMatch(Objects::isNull);
+    public static void requireNonNull(Collection<?> items) {


Giving this the same name as Objects#requireNonNull may be dangerous though. If import static seedu.address.commons.util.CollectionUtil.requireNonNull is not present in the file, then Objects#requireNonNull would be used instead, which would be hard to detect by just reading the diff in a code review.

Maybe give this another name? Maybe requireNonNullElements or requireAllNonNull.

Zhiyuan-Amos · 2017-05-30T08:28:29Z

@MightyCupcakes any comments? :>

MightyCupcakes · 2017-05-30T11:41:47Z

No other comments.

Java’s native Objects#requireNonNull(T) can only take in an Object. Calling the native method requires multiple calls in cases where certain methods require multiple parameters to be non-null. Let’s update CollectionUtil#isAnyNull(…) to do the same operation as Objects#requireNonNull(T), and also able to take in multiple parameters.

CanIHasReview-bot · 2017-05-30T12:43:21Z

v10

@Zhiyuan-Amos submitted v10 for review.

[v10 1/2] Replace asserts with proper exceptions for parameter validation
[v10 2/2] Update CollectionUtil#isAnyNull(Object...) to throw NPE

(📚 Archive) (📈 Interdiff between v9 and v10)

Checkout this PR version locally

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

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

Zhiyuan-Amos · 2017-06-01T00:55:43Z

@MightyCupcakes ping :>

Zhiyuan-Amos force-pushed the 239-reduce-asserts branch from 1087d98 to c1d7c23 Compare May 16, 2017 03:27

Zhiyuan-Amos requested a review from yamgent May 16, 2017 03:34

CanIHasReview-bot added the s.ToReview label May 16, 2017

yamgent reviewed May 16, 2017

View reviewed changes

Zhiyuan-Amos force-pushed the 239-reduce-asserts branch 3 times, most recently from 4f67e03 to 9617027 Compare May 16, 2017 07:46

yamgent approved these changes May 16, 2017

View reviewed changes

yamgent requested review from pyokagan and MightyCupcakes May 16, 2017 08:03

yamgent requested a review from damithc May 18, 2017 03:01

pyokagan removed their request for review May 18, 2017 05:17

pyokagan reviewed May 18, 2017

View reviewed changes

Zhiyuan-Amos force-pushed the 239-reduce-asserts branch 4 times, most recently from 2295b00 to fc13a60 Compare May 19, 2017 05:32

Zhiyuan-Amos requested a review from limmlingg May 19, 2017 05:34

limmlingg suggested changes May 19, 2017

View reviewed changes

Zhiyuan-Amos force-pushed the 239-reduce-asserts branch from 4228875 to a4fa2db Compare May 29, 2017 07:09

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels May 29, 2017

yamgent approved these changes May 30, 2017

View reviewed changes

MightyCupcakes suggested changes May 30, 2017

View reviewed changes

Zhiyuan-Amos force-pushed the 239-reduce-asserts branch from a4fa2db to fc4b49f Compare May 30, 2017 05:03

CanIHasReview-bot requested a review from MightyCupcakes May 30, 2017 05:05

Zhiyuan-Amos force-pushed the 239-reduce-asserts branch from fc4b49f to 202c824 Compare May 30, 2017 06:12

pyokagan reviewed May 30, 2017

View reviewed changes

Zhiyuan-Amos force-pushed the 239-reduce-asserts branch 2 times, most recently from 1ac1e31 to 931511e Compare May 30, 2017 12:20

Zhiyuan-Amos force-pushed the 239-reduce-asserts branch from 931511e to 288a958 Compare May 30, 2017 12:22

Zhiyuan-Amos mentioned this pull request May 30, 2017

Allow user to see the command history #439 #440

Merged

MightyCupcakes approved these changes Jun 1, 2017

View reviewed changes

Zhiyuan-Amos added s.ToMerge and removed s.ToReview labels Jun 1, 2017

yamgent merged commit 2f5b10f into se-edu:master Jun 1, 2017

Zhiyuan-Amos deleted the 239-reduce-asserts branch June 1, 2017 23:27

Zhiyuan-Amos mentioned this pull request Jul 13, 2017

Assumption: Add assertAllNotNull(Object... objects) TEAMMATES/teammates#7730

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce unnecessary asserts #239 #414

Reduce unnecessary asserts #239 #414

Zhiyuan-Amos commented May 15, 2017 •

edited

Loading

CanIHasReview-bot commented May 16, 2017

yamgent left a comment •

edited

Loading

yamgent left a comment

yamgent May 16, 2017

Zhiyuan-Amos May 16, 2017 •

edited

Loading

yamgent May 16, 2017

yamgent left a comment

CanIHasReview-bot commented May 16, 2017

yamgent commented May 17, 2017

pyokagan commented May 18, 2017

pyokagan commented May 18, 2017

Zhiyuan-Amos commented May 18, 2017

pyokagan commented May 18, 2017

pyokagan May 18, 2017

pyokagan May 18, 2017

Zhiyuan-Amos May 19, 2017 •

edited

Loading

yamgent May 19, 2017

pyokagan commented May 18, 2017

CanIHasReview-bot commented May 19, 2017

limmlingg May 19, 2017

CanIHasReview-bot commented May 29, 2017

MightyCupcakes left a comment

MightyCupcakes left a comment

CanIHasReview-bot commented May 30, 2017

CanIHasReview-bot commented May 30, 2017

Zhiyuan-Amos commented May 30, 2017

pyokagan May 30, 2017

Zhiyuan-Amos May 30, 2017

Zhiyuan-Amos commented May 30, 2017

MightyCupcakes commented May 30, 2017

CanIHasReview-bot commented May 30, 2017

Zhiyuan-Amos commented Jun 1, 2017

Reduce unnecessary asserts #239 #414

Reduce unnecessary asserts #239 #414

Conversation

Zhiyuan-Amos commented May 15, 2017 • edited Loading

CanIHasReview-bot commented May 16, 2017

v1

yamgent left a comment • edited Loading

Choose a reason for hiding this comment

yamgent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhiyuan-Amos May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamgent left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented May 16, 2017

v2

yamgent commented May 17, 2017

pyokagan commented May 18, 2017

pyokagan commented May 18, 2017

Zhiyuan-Amos commented May 18, 2017

pyokagan commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhiyuan-Amos May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyokagan commented May 18, 2017

CanIHasReview-bot commented May 19, 2017

v3

Choose a reason for hiding this comment

CanIHasReview-bot commented May 29, 2017

v7

MightyCupcakes left a comment

Choose a reason for hiding this comment

MightyCupcakes left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented May 30, 2017

v8

CanIHasReview-bot commented May 30, 2017

v9

Zhiyuan-Amos commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhiyuan-Amos commented May 30, 2017

MightyCupcakes commented May 30, 2017

CanIHasReview-bot commented May 30, 2017

v10

Zhiyuan-Amos commented Jun 1, 2017

Zhiyuan-Amos commented May 15, 2017 •

edited

Loading

yamgent left a comment •

edited

Loading

Zhiyuan-Amos May 16, 2017 •

edited

Loading

Zhiyuan-Amos May 19, 2017 •

edited

Loading