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 throw
n 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 all1, then calling UpdateUser
with null
as userArg
is a contract violation and your implementation is allowed to do literally anything. “Anything” of course includes throw
ing 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 circumstances2, then simply throw
ing 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.
1 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.
2 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.
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