I agree with @MartinMaat about picking your battles.
There are many cases of "just in case" due to not really understanding the language despite the language being fixed in its rules for many of these things -- over parenthesizing an expression that doesn't need it due to not understanding the precedence rules of the language. But still, such practice is mostly harmless.
When I was younger, I felt that we should learn the details of the language and thus avoid writing such superfluous code. (One of my pet-peeves was return (0); with its unnecessary parens.) However, I now moderate that position, in particular, because we use so many different languages now, jumping from client to server, etc... So, I now cut some slack for some such issues.
You're point about cyclomatic starts to go to logically reasoned argument. Let's look at Code Coverage and especially higher levels of coverage:
- Each decision takes every possible outcome
Since we cannot force the new operation to return NULL, there's no way to reach higher levels of code coverage for this conditional operation. Of course, this may or may not be important to your organization!
However, because of this code coverage issue I would prioritize it higher than than over-parenthesizing.
On the other hand, the underlying generated code will probably not suffer one bit for this as the code generations, JIT, and optimizers all understand that a newed value will never be null. So, the real cost comes only in terms of readability and source code coverage capabilities.
I would ask you what does the "else-part" of such an if statement look like?
If there is no else-part, I would argue that simply falling off the end of the routine or falling through to other code (i.e. no else for this if) is potentially dangerous, since now this "just in case" logically suggests that callers and/or further code down the line handles NULL as well.
If it reads:
p = new Object ();
if ( p != null ) {
p.field = value;
}
else {
throw new NullReferenceException ();
}
then this is really overkill, as the language does all of that for us.
I might suggest reversing the sense of the conditional — perhaps your colleague will be more comfortable with this:
p = new Object ();
if ( p == null ) {
throw new NullReferenceException ();
}
else {
p.field = value;
}
Now you can argue for the removal of the else wrapper, since it is very clearly unnecessary:
p = new Object ();
if ( p == null ) {
throw new NullReferenceException ();
}
p.field = value;
With this, the "just in case" is now what is conditional, whereas the succeeding code isn't. This approach further reinforces that when allocation fails, the appropriate response is throwing, rather than continuing to run code in this method and/or in this call chain (without any other proper handling of the allocation failure).
So, in summary there are two logically reasoned arguments to make here against this practice:
- Higher code coverage levels cannot be reached as we cannot force out of memory (or any constructor failure) to return null.
- The "just in case" (as shown above in the question) is incomplete and as such is flawed because of the inconsistency in expectations of how null were to be handled by other code beyond/past the
p.field = value;.
Fundamentally, it seems like perhaps your colleague is on the fence about using exceptions — even though there's no choice here in C# for such things. (If we want well-tested code we cannot code for both an exception model for handling null and a non-exception model using null-return-values, side-by-side.) Perhaps if you reason with your colleague through these topics, they'll see some light!
