Assumption: Add assertAllNotNull(Object... objects)

[Zhiyuan-Amos]

Assumption has a method assertNotNull(Object).

This method has to be called multiple times if the caller has multiple objects that should not be null. This leads to unnecessary code verbosity.
For example, in Logic#createAccount(String, String, boolean, String, String, StudentProfileAttributes), there are 5 calls of assertNotNull(Object).

Assumption.assertNotNull(googleId);
Assumption.assertNotNull(name);
Assumption.assertNotNull(isInstructor);
Assumption.assertNotNull(email);
Assumption.assertNotNull(institute);

Let's create a helper method assertAllNotNull(Object... objects) to resolve the issue of unnecessary code verbosity.

[LiHaoTan]

How about we use Preconditions from Guava instead? I personally think our Assumption is not very well designed, for example, one of the problems is what you pointed out.

For instance, Guava tackles this problem by doing this instead.

this.googleId = Preconditions.checkNotNull(googleId);
this.name = Preconditions.checkNotNull(name);
this.isInstructor = Preconditions.checkNotNull(isInstructor);
this.email = Preconditions.checkNotNull(email);
this.institute = Preconditions.checkNotNull(institute);

See: https://github.com/google/guava/wiki/PreconditionsExplained

Not to mention we can do this.field = checkNotNull(field) to make it more concise and it still very clear what we are doing.

See

After static imports, the Guava methods are clear and unambiguous. checkNotNull makes it clear what is being done, and what exception will be thrown.

Furthermore,

We recommend that you split up preconditions into distinct lines, which can help you figure out which precondition failed while debugging. Additionally, you should provide helpful error messages, which is easier when each check is on its own line.

I think this happened because we tried to reinvent the wheel, and did not consider the enormous work others have put into addressing the null problem.

The other thing to notice is that Preconditions.checkNotNull throws a NullPointerException, not an assertion and not a IllegalArgumentException.

See

Preconditions.checkArgumentNotNull() (throws IllegalArgumentException)

We realize there are many valid arguments in favor of throwing IAE on a null argument. In fact, if we had a time machine to go back >15 years, we might even try to push things in that direction. However, we have decided to stick with the JDK and Effective Java preference of NullPointerException.

If you're steadfast in your belief that IAE is right, you still have checkArgument(arg != null), just without the convenience of it returning arg, or you can create a local utility to your project.

Source: https://github.com/google/guava/wiki/IdeaGraveyard#preconditionscheckargumentnotnull-throws-illegalargumentexception

Here’s the excerpt from Effective Java

Arguably, all erroneous method invocations boil down to an illegal argument or illegal state, but other exceptions are standardly used for certain kinds of illegal arguments and states. If a caller passes null in some parameter for which null values are prohibited, convention dictates that NullPointerException be thrown rather than IllegalArgumentException. Similarly, if a caller passes an out-of-range value in a parameter representing an index into a sequence, IndexOutOfBoundsException should be thrown rather than IllegalArgumentException

And a longer discussion on Guava’s issue tracker, google/guava#1648.
And if we trace through the Stack Overflow discussion, we find arguments for both sides. However, one thing to note is that Java has standardized the meaning of NullPointerException, which is also perhaps why Effective Java recommended it.

From NullPointerException JavaDocs

Applications should throw instances of this class to indicate other illegal uses of the null object.

Also introduced in Java 7,

Checks that the specified object reference is not null. This method is designed primarily for doing parameter validation in methods and constructors, as demonstrated below:
public Foo(Bar bar) {
this.bar = Objects.requireNonNull(bar);
}

which also throws a NullPointerException.

https://docs.oracle.com/javase/7/docs/api/java/util/Objects.html#requireNonNull(T)

And here's Guava's take on assertions

A conventional assertion is a check that should only fail if the class itself (that contains the check) is broken in some way. (In some cases this can extend to the package.) These can take various forms including postconditions, class invariants, and internal preconditions (on non-public methods).

Then another problem would be excessive null checks, I think it's probably better to let things fail if a NullPointerException will happen unless when not checking for a null can result in unexpected behavior or we want to fail fast.

@Zhiyuan-Amos, what do you think?

[Zhiyuan-Amos]

Then another problem would be excessive null checks, I think it's probably better to let things fail if a NullPointerException will happen unless when not checking for a null can result in unexpected behavior or we want to fail fast.

Generally, I think it's good that we standardise things to fail fast. This means that there will be a lot of null checks over the entire codebase, but it ensures that the scenario you mentioned above (when not checking for a null can result in unexpected behavior) does not happen. Sometimes, it's hard to tell whether not checking for null can result in unexpected behaviour as you really have to eyeball each line of code (which isn't really safe imo). So, the safest way and most bug-free way to do it is to do null/precondition checks at the start of each method.

The debatable one would actually be in cases of overloaded methods:

public void foo(String bar) {
    // should we do assertNotNull(bar) here?
    foo(bar, "faz");
}

public void foo(String bar, String faz) {
    assertNotNull(bar);
    assertNotNull(faz);
    // do something
}

The check at this point in time isn't necessary since null check for bar is done in the overloaded method. However, there may be cases whereby we edit foo(String):

public void foo(String bar) {
    // not calling overloaded method anymore
    // do something
}

That way, the developer may forget to do the precondition check. (I think we generally forget do precondition checks, myself included :P)

Also, between using Guava#checkNotNull vs Objects#requireNonNull, one benefit is that Guava has a checkArgument(boolean) method, but Java Objects does not have such a method. So for the purpose of standardizing the Preconditions check import, I think using Guava would be better.

I think it doesn't matter whether IAE or NPE are thrown, since they aren't meant to be handled right? (At least for SE-EDU).

[Zhiyuan-Amos]

@LiHaoTan we discussed more f2f, and we even discussed where should we put assertions (whether they should be in the entire code base, or only in components such as Logic).

I think that should not be the focus of this issue. As you have pointed out, this issue can simply be about removing Assumption class and migrating the existing precondition checks to use Guava instead. As such, we should not add new assertions/precondition checks.

Perhaps the next thing to decide on is whether we should implement our own checkAllNonNull, so that we can shorten code.

For example:

public void createAccount(String googleId, String name, boolean isInstructor, String email, String 
    institute, StudentProfileAttributes studentProfileParam) throws InvalidParametersException {
    Assumption.assertNotNull(googleId);
    Assumption.assertNotNull(name);
    Assumption.assertNotNull(isInstructor);
    Assumption.assertNotNull(email);
    Assumption.assertNotNull(institute);
 
    StudentProfileAttributes studentProfile = studentProfileParam;
    if (studentProfile == null) {
        studentProfile = new StudentProfileAttributes();
        studentProfile.googleId = googleId;
    }
    AccountAttributes accountToAdd = new AccountAttributes(googleId, name, isInstructor, email, 
        institute, studentProfile);

    accountsLogic.createAccount(accountToAdd);
}

Over here we can't do what you suggested earlier:

this.googleId = Preconditions.checkNotNull(googleId);
this.name = Preconditions.checkNotNull(name);
this.isInstructor = Preconditions.checkNotNull(isInstructor);
this.email = Preconditions.checkNotNull(email);
this.institute = Preconditions.checkNotNull(institute);

because we are not assigning the attributes, rather we are passing it into a constructor.

As such, calling something like:
checkAllNonNull(googleId, name, email, institute, studentProfileParam) would shorten the code from 5 lines to 1 line.

The con, however, is that we can't tell immediately which variable is null.
Another con is that you can't pass error messages into this method (however in the existing code base, at least for Logic, it seems that error messages aren't passed in that frequently, so this isn't much of an issue).

[LiHaoTan]

I agree with you that it is probably not so important that we can tell immediately which variable is null. I also think that it is fine not to be able to pass error messages into checkAllNonNull since we can always use the single arg method to check for null.

However, I would think that there needs to be a stronger reason than just reducing the number of lines to add checkAllNonNull since it contradicts with the original intent where we want to separate them out to different lines so we know exactly which line the NullPointerException originated from.

Guava also mentions

We recommend that you split up preconditions into distinct lines, which can help you figure out which precondition failed while debugging.

I'm not saying that we should take Guava as the canonical source though but we probably should consider more if we should add checkAllNonNull.

So going back to the checkNonNull problem, instead of referring to your example here, let's refer to the fictitious example below because I think that is a badly designed method so we cannot address the problems of null checking properly here (for instance this method actually allowed null as a parameter). I also stripped isInstructor before we end up writing more nonsensical code.

public void createAccount(String googleId, String name, String email, String 
    institute, StudentProfileAttributes studentProfileParam) throws InvalidParametersException {
    Assumption.assertNotNull(googleId);
    Assumption.assertNotNull(name);
    Assumption.assertNotNull(email);
    Assumption.assertNotNull(institute);
    Assumption.assertNotNull(studentProfileParam);
 
    AccountAttributes accountToAdd = new AccountAttributes(googleId, name, email, 
        institute, studentProfile);
    AccountPerformanceIndexes accountPerformanceIndexes = new AccountAttributes(googleId, 
       name, email, institute, studentProfile);
 
    accountsLogic.createAccount(accountToAdd, accountPerformanceIndexes);
}

In this case, we would have to assume the two constructors are perhaps package-private so they don't check for nulls themselves and in order not to repeat code we do the null check here.

However, we could have simply had something as follows:

    AccountAttributes accountToAdd = new AccountAttributes(checkNotNull(googleId), checkNotNull(name), checkNotNull(email), checkNotNull(institute), checkNotNull(studentProfile));
    AccountPerformanceIndexes accountPerformanceIndexes = new AccountAttributes(checkNotNull(googleId), checkNotNull(name), checkNotNull(email), checkNotNull(institute), checkNotNull(studentProfile));

Though that is repeating code so maybe we are just better off

    checkNotNull(googleId);
    checkNotNull(name);
    checkNotNull(email);
    checkNotNull(institute);
    checkNotNull(studentProfileParam);

    AccountAttributes accountToAdd = new AccountAttributes(googleId, name, email, 
        institute, studentProfile);
    AccountPerformanceIndexes accountPerformanceIndexes = new AccountAttributes(googleId, 
       name, email, institute, studentProfile);

So perhaps one compelling reason for adding checkAllNonNull would be this scenario is very common, but I would think that is not the case because we should probably have error messages when we are checking so many arguments (except maybe in uncommon scenarios like this).

Though another issue is that should we just migrate all precondition checks to use Guava instead?

Because take for example FeedbackRubricQuestionDetails.RubricStatistics#calculatePercentageFrequencyAndAverage

We should probably use an assert instead isn't it? Anyway, the method should have been private, not package-private.

void calculatePercentageFrequencyAndAverage() {
            Assumption.assertNotNull("Response Frequency should be initialised and calculated first.",  responseFrequency);

Also, the other thing is I don't know when assertions were illegal in App Engine, probably a long time ago and we should be migrating them to proper assertions instead.

[LiHaoTan]

With regards to IAE and NPE, I think it matters because it helps to immediately tell how the error occurred.

For example, when we see IAE, we immediately know that an illegal argument is passed. And when we see NPE, we know that null is being passed, and usually, that occurs from some error such as a problematic computation.

However, we would not be able to tell when one specifically passes null into a method, in that case it would be more like an illegal argument.

[damithc]

@LiHaoTan you think we should switch to Guava?
@Zhiyuan-Amos are we using Guava already for some other thing?

[Zhiyuan-Amos]

Quote from Guava:

We recommend that you split up preconditions into distinct lines, which can help you figure out which precondition failed while debugging.

This I agree. I'm wondering whether we can have a workaround. That is, whenever we encounter NPE, we log it down first before throwing NPE. That way, we can effectively find out which object caused the error (just have a counter to count which item in the varargs that it's checking). That way we need our own implementation of checkNotNull though hmm. (It's simple to implement though).

Though another issue is that should we just migrate all precondition checks to use Guava instead?

Because take for example FeedbackRubricQuestionDetails.RubricStatistics#calculatePercentageFrequencyAndAverage

We should probably use an assert instead isn't it? Anyway, the method should have been private, not package-private.

Yup, sorry that I didn't specify myself clearer. We should only migrate precondition checks for public methods to use Guava, and private methods to remain using assertions.

With regards to IAE and NPE, I think it matters because it helps to immediately tell how the error occurred.

Yup this I definitely agree. In Guava there's checkArgument(boolean) which throws IAE, and checkNotNull(T) which throws NPE. We can use these 2 methods.

@damithc
For AddressBook-Level4, we are not using Guava. Paul mentioned in se-edu/addressbook-level4#414 (comment):

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().

However, Paul's comment isn't applicable for Teammates :)

[LiHaoTan]

@damithc yes I think we should use Guava since we have already included it but we are not making much use of it.

@Zhiyuan-Amos

This I agree. I'm wondering whether we can have a workaround. That is, whenever we encounter NPE, we log it down first before throwing NPE. That way, we can effectively find out which object caused the error (just have a counter to count which item in the varargs that it's checking). That way we need our own implementation of checkNotNull though hmm. (It's simple to implement though).

Yea this can be a possible approach. As per we discussed this will require additional maintenance, e.g. writing tests for such a method so since scenarios such as the two constructor scenario are currently rare in the code, the cost required to create such a method may not be worthwhile.

Maybe we can add it if we do find a compelling reason to add such a method in the future.

Yup, sorry that I didn't specify myself clearer. We should only migrate precondition checks for public methods to use Guava, and private methods to remain using assertions.

So for this issue, shall we focus on what you recommend here? :)

[wkurniawan07]

Closing as explained in #7996.