What's the point of Guava checkNotNull [duplicate]

Question

I'm pretty new to Guava (let's be honest, I'm not "pretty new", I'm a complete rookie on that subject) and so I decided to go through some documentation and got quite amazed while reading this:

com.google.common.base.Preconditions.checkNotNull(...)

I don't get the point of this method. This means that instead of doing :

myObject.getAnything();

(which might cause a NullPointerException if myObject is null)

I should use

checkNotNull(myObject).getAnything();

which will throw a NullPointerException if myObject is null and return myObject if it is not null.

I'm puzzled and this might be the stupidest question ever but ...

What is the point of this? Those two lines do the exact same thing as for outcomes given any situations I can think of.

I don't even think that the latter is more readable.

So I must be missing something. What is it?

Pooh

I don't really buy in the idea of adding checkNotNull at every place. IMHO, it'd be more preferable if you verify your input explicitly, instead of over-relying on the syntactic sugar that help obfuscate your code. Fail fast is OK, but checkNotNull just doesn't give any other info for troubleshooting --> you can simply make your own exception for wrong input format, and put in a nice message explaining the specific situation – Amargo

I totally agree this is ridiculous. Checking for null and then throwing an NPE yourself is, in my humble opinion totally nuts. At least throw a ValidationException or IllegalArgumentException. – Driftage

Consider providing better diagnostics with checkNotNull(reference, errorMessageTemplate, errorMessageArgs): guava.dev/releases/23.0/api/docs/com/google/common/base/… – Verbid

Answer 1

The idea is to fail fast. For instance, consider this silly class:

public class Foo {
    private final String s;

    public Foo(String s) {
        this.s = s;
    }

    public int getStringLength() {
        return s.length();
    }
}

Let's say you don't want to allow null values for s. (or else getStringLength will throw a NPE). With the class as-is, by the time you catch that null, it's too late -- it's very hard to find out who put it there. The culprit could well be in a totally different class, and that Foo instance could have been constructed a long time ago. Now you have to comb over your code base to find out who could possibly have put a null value there.

Instead, imagine this constructor:

public Foo(String s) {
    this.s = checkNotNull(s);
}

Now, if someone puts a null in there, you'll find out right away -- and you'll have the stack trace pointing you exactly to the call that went wrong.

---

Another time this can be useful is if you want to check the arguments before you take actions that can modify state. For instance, consider this class that computes the average of all string lengths it gets:

public class StringLengthAverager {
    private int stringsSeen;
    private int totalLengthSeen;

    public void accept(String s) {
        stringsSeen++;
        totalLengthSeen += s.length();
    }

    public double getAverageLength() {
        return ((double)totalLengthSeen) / stringsSeen;
    }
}

Calling accept(null) will cause an NPE to get thrown -- but not before stringsSeen has been incremented. This may not be what you want; as a user of the class, I may expect that if it doesn't accept nulls, then its state should be unchanged if you pass a null (in other words: the call should fail, but it shouldn't invalidate the object). Obviously, in this example you could also fix it by getting s.length() before incrementing stringsSeen, but you can see how for a longer and more involved method, it might be useful to first check that all of your arguments are valid, and only then modify state:

    public void accept(String s) {
        checkNotNull(s); // that is, s != null is a precondition of the method

        stringsSeen++;
        totalLengthSeen += s.length();
    }

Southerland

Shouldn't it be more this.s=Preconditions.checkNotNull(s); – Pooh

but yeah I did not think about the storing case. – Pooh

The typical pattern is this.s = checkNotNull(s); with a static import. – Keven

@Pooh I added another use case. – Southerland

Personally I think this kind of error reporting should be avoided. Whatever trick you do, a NullPointerException literally has little to no meaning. I'm strongly against passing/returning null in ANY function (unless you're working with legacy API), therefore any effort to do so should get a WrongApiUsage exception into the face. – Amargo

@HoàngLong I also wish that checkNotNull would throw an IllegalArgumentException, instead of NPE. To me, an NPE means that someone tried to dereference a null pointer -- not that someone caught the fact that it was null before they tried to dereference it. But with that said, I also think that "IllegalArgumentException: foo was null" isn't more helpful than "NullPointerException: foo" in any practical sense. So, while I kinda wish that the Guava folks had picked IllegalArgumentException, the convenience and standardization of checkNotNull override that nit, at least for me. – Southerland

@Southerland if you want an IllegalArgumentException, you can use checkArgument instead of checkNotNull. checkNotNull has a "good enough" meaning if you use the implementation with a string explanation. It's intended to be a safeguard, not a logical check. – Yand

Answer 2

myObject.getAnything(); (which might cause a NullPointerException if myObject is null)

No... it will throw NPE whenever myObject == null. In Java, there's no chance of calling a method with null receiver (a theoretical exception are static methods, but they can and should be always called without any object).

---

I should use checkNotNull(myObject).getAnything();

No you should not. This would be rather redundant (Update).

You should use checkNotNull in order to fail fast. Without it, you may pass an illegal null to another method, which passes it further, and so on and so on, where it finally fails. Then you can need some good luck to find out that actually the very first method should have refused null.

---

The answer by yshavit mentions an important point: Passing an illegal value is bad, but storing it and passing it later is even worse.

Update

Actually,

 checkNotNull(myObject).getAnything()

makes sense, too, as you clearly express your intent to not accept any nulls. Without it, someone could think that you forgot the check and convert it into something like

 myObject != null ? myObject.getAnything() : somethingElse

OTOH, I don't think the check is worth the verbosity. In a better language, the type system would consider nullability and give us some semantic sugar like

 myObject!!.getAnything()                    // checkNotNull
 myObject?.getAnything()                     // safe call else null
 myObject?.getAnything() ?: somethingElse    // safe call else somethingElse

for nullable myObject, while the standard dot syntax would be allowed only when myObject is known to be non-null.

Susi

So that is like checking in the begin of a method that a precondition about the method is valid? – Elbertelberta

Exactly. Checking for illegal arguments immediately ensures you wan't have to hunt them down later. Moreover, everyone looking at the method sees what's forbidden (sure, it should be documented...). – Susi

@Juru: Yep, that's why it's in the Preconditions class! – Keven

Answer 3

I have read this whole thread few minutes ago. Nevertheless I was confused why should we use checkNotNull. Then look over Precondition class doc of Guava and I got what I expected. Excessive use of checkNotNull will degrade performance definitely.

My thought is checkNotNull method is worth required for data validation which comes from user directly or very end API to user interaction. It shouldn't be used in each and every methods of internal API because using it you can't stop exception rather correct your internal API to avoid Exception.

According to DOC: Link

Using checkNotNull:

public static double sqrt(double value) {
     Preconditions.checkArgument(value >= 0.0, "negative value: %s", value);
     // calculate the square root
}

Warning about performance

The goal of this class is to improve readability of code, but in some circumstances this may come at a significant performance cost. Remember that parameter values for message construction must all be computed eagerly, and autoboxing and varargs array creation may happen as well, even when the precondition check then succeeds (as it should almost always do in production). In some circumstances these wasted CPU cycles and allocations can add up to a real problem. Performance-sensitive precondition checks can always be converted to the customary form:

if (value < 0.0) {
     throw new IllegalArgumentException("negative value: " + value);
}

Phraseologist

Giving -1, sorry: I don't recommend using this for user data validation because you will want to give the user a more user-friendly non-technical error message. Also runtime exceptions are not intended in case the erratic situation can be expected to happen. A user can be expected to make a mistake now and then. I'd suggest checking method arguments in public methods but not in private methods. Catching programming errors earlier is worth more than premature optimization. Replace the Precondition check only when you find it to be an actual performance problem. – Epigenesis

@HennoVermeulen As you said "checking method arguments in public methods" I also told "It shouldn't be used in each and every method of internal API". Thanks – Phraseologist

Upvoted for the heads up on on performance, but as others have said, not a great approach to user validation! – Exponible