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.
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
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, the behavior is undefined.
or simply doesn't say anything at all1, then calling
userArg is a contract violation and your implementation is allowed to do literally anything. “Anything” of course includes
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.
The second check in your code is very different. Here, you're checking that
null. This is strange.
Either the documentation of
Get says that it doesn't
null, then you can rely on it and don't have to check, or it says that it might
null under certain circumstances2, then simply
NullPointerException is probably not the right reaction.
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
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
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.