Benefit of throwing exceptions for null parameters

Question

The codebase I work with has a certain pattern prevalent in all public methods, which goes like this:

public void UpdateUser(User userArg)
{
 Framework.NullCheck(userArg);
 var user = userDb.Get(userArg.Id);
 Framework.NullCheck(user);
 user = userArg;
 userDb.Update(user);
}

The NullCheck method is generic and will check if the object equals null. If it does, a NullReferenceException is thrown with a generic message.

I have never thought to ask my colleagues what the benefits of doing this are.

Considering that this code is executing within an application where all user input is sanitised (including null checks) at both client side and server side as soon as it is received, and it is stored in a database with the correct constraints on nulls, doing a check like this within every method seems like overkill. Not to mention it adds just that little bit more time overhead when creating a new method, which all adds up.

• gnat: Possible duplicate of A very basic question about whether I should check for null and throw NPE?
• Richard Tingle: A stray null can travel a long way from where it was incorrectly passed before blowing up. It's annoying that the java language itself allows all variables to potentially contain null. All methods seems a little excessive but I often put these in while debugging an NPE, having put them in taking them out seems unhelpful
• Frayt: @gnat I'd like to get some more opinions; the answers in that question seem to be against the null checks and I'd like to hear some for null checks arguments
• Frayt: @RichardTingle With a stack trace do you think that is still a problem?
• Richard Tingle: @Fray yes, stack trace helps sometimes but not always. Imagine a physics engine. setCollisionHandler(null) It may be minutes, perhaps even hours before the NPE occures because the null sits as invalid state until it's exercised, and at that point the stack trace will be for what validly called the handler, not what invalidly put the null in

Answer 1

I don't think that the linked question has an answer that addresses your specific case so I'll post another answer here.

The code snippet you've shown contains two calls to Framework.NullCheck that have a very different purpose.

Parameters

The first one validates a parameter. Do we need to test it? The first question here should be what the contract of the UpdateUser function says. You haven't shown its documentation so I don't know.

Interfaces with wide contracts

If the documentation says

If userArg is null, a NullPointerException is thrown and the function has no effect.

then yes, you absolutely have to check it. We say that such an interface has a wide contract. Well, in this case, the check is actually redundant anyway because if userArg == null then the very first line of code

var user = userDb.Get(userArg.Id);

will do exactly what the contract advertises because the JVM will first evaluate the function parameters and evaluating userArg.Id is guaranteed to trigger a NullPointerException without any side effects. You could still argue that the explicit check is cleaner because it makes it more obvious and if you ever edit the implementation, you're less likely to accidentally break your contract.

Interfaces with narrow contracts

If, on the other hand, the documentation says

userArg must not be null.

or

If userArg is null, the behavior is undefined.

or simply doesn't say anything at all¹, then calling UpdateUser with null as userArg is a contract violation and your implementation is allowed to do literally anything. "Anything" of course includes throwing a NullPointerException without having any side-effects. And doing so might be a good idea in terms of defensive programming. However, I would not optimize for the failure case. Assume that most of the time, your function will be called with valid arguments. (If this isn't true, there is something else to worry more about.) In the rare case where it is called out-of-contract, first doing some work and then failing is okay. Especially if "some work" doesn't have visible side-effects.

Return values

The second check in your code is very different. Here, you're checking that userDb.Get(userArg.Id); doesn't return null. This is strange.

Either the documentation of Get says that it doesn't return null, then you can rely on it and don't have to check, or it says that it might return null under certain circumstances², then simply throwing a NullPointerException is probably not the right reaction.

Conclusion

I would keep those checks for parameters in every function that has a wide contract, even if it might not be strictly necessary, just because it makes the code more obvious.

In functions with a narrow contract, I'd use null checks sparingly when the performance overhead seems acceptable and the function would do potentially harmful stuff (side effects) before failing anyway. I'd probably prefer using assert statements in this case, though. This makes it clearer that you're doing a defensive check that will only be triggered in case of a contract violation.

I would get rid of all checks for return values.

---

_{¹ Whether not saying anything about a parameter implies that it must not be null might be open to debate. Because null rarely is a valid value, I prefer to explicitly state where it is allowed and otherwise remain silent. You should consult your project's conventions about this to be sure, though.}

_{² For example, it might use the return value null to indicate that the requested entry was not found in the database. This might not be the best design choice but such code does exist in reality.}

• Frayt: Great answer. Unfortunately my codebase has zero technical documentation so contracts aren't present. To clarify: Yes, null will be returned from userDb.Get if there is no matching entry in the database.
• 5gon12eder: @Frayt Okay, but is a NullPointerException really what you want in this case? Wouldn't is be better to throw a (checked?) UnknownUserException or return a status code from UpdateUser? A non-existing user seems like a fairly normal case that shouldn't crash your application – which an NPE usually does (or, at least, should, as it indicates a bug).

Answer 2

The good old saying goes like "Crash early, crash often".

It makes debugging much easier to get instant feedback on problems. If the codebase is not (削除) littered with documentation (削除ここまで) well documented, those checks will make it immediately clear to the reader what is expected.

One could argue if assertions wouldn't be a better choice - but the answer might well be "it depends".

Checking return values seems usually wrong to me, though. Typically the called function should already have thrown an exception. But this really depends on the context - it may be perfectly fine for a user-lookup function to return null for any non-existing user names, but in that specific scenario you should be guaranteed a valid user and the non-existance indicates a bug...

• Giorgio: "The good old saying goes like "Crash early, crash often".", well... it depends. Try doing this in an air traffic control software.
• 5gon12eder: @Giorgio It is much better – especially – for a faulty air traffic control system to crash and hand over to the backup system than to continue producing nonsensical output.
• Eiko: @Georgio Like returning "sensible" default data, i.e. isRunwayUsed() simply returning 0 for errors? ;-) Yes, it always depends. But it often just drives the system deeper and deeper into chaos - making it harder and harder to recover later.
• Giorgio: The alternative to a crash is to let the system stop as soon as an anomaly is detected and ask for user intervention to repair or restart it. Crashing means the programmers made little effort to let the system recover properly. It is OK for the GUI of a web application. It is a recipe for disaster in mission-critical software.
• Giorgio: @Elko: We seem to agree: it depends.

Answer 3

The problem is not that you do explicitly check something for null.

The problem you have is:

The NullCheck method is generic and will check if the object equals null. If it does, a NullReferenceException is thrown with a generic message.

IFF you explicitly check, throw ArgumentNullException for the parameter with the name of the method and the parameter.

In the case of the second NullCheck, since you wrote

To clarify: Yes, null will be returned from userDb.Get if there is no matching entry in the database.

throwing a NullRefExc here is really horrible - at the very least throw an InvalidOperationException indicating which ID hasn't been found in the the DB and that passing invalid IDs is an error for this specific method.