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.

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.}

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

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.