8

I'm working on a code base with a coworker who has a habit of checking the results of a constructor for a null in a fashion similar to this

Person p = new Person();
if (p != null)
{
    p.Name = "John Smith";
}

My understanding of the .NET landscape is that a constructor will never leave an assignment unfulfilled unless an exception is thrown. So in the case above, the null check is useless. Either p is allocated, or an exception will be thrown causing the property setter to be skipped.

I have asked my coworker about this in passing, and I get a passive answer along the lines of "just in case". I don't like this kind of "paronia-programming" myself. I think it hurts the readability and needlessly increases cyclomatic complexity. Because of this, I would like to make a formal request that this practice be stopped. Is this a reasonable request? Am I missing something?

  • 6
  • 1
    Seems like it should be a compiler warning at a minimum, or a code analysis warning. – Frank Hileman Aug 23 '18 at 21:14
  • @FrankHileman - good point... the C# compiler does have a warning (0161) about unreachable code, although I'm not 100% certain it's able to detect this specific case. – Jules Aug 23 '18 at 22:14
  • is it really a standard practice to have the check, or is it just left over from a previous version? which maybe had p = Repository.Get(123) or something – Ewan Aug 30 '18 at 14:36
12

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:

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

  1. Higher code coverage levels cannot be reached as we cannot force out of memory (or any constructor failure) to return null.
  2. 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!

  • You hit the nail on the head with regards to my colleague being on the fence about exceptions. To you question, there is no else condition in my example. What they would do is just omit the work if the value is null. And that null check would be propagated up if the allocated instance is returned. It smells like error codes, right? I didn't see this case as related to that issue, but you have correctly pointed out that they are very much related. Thanks for the comprehensive response. The code coverage issue is a good take away for me. – Jason Tyler Aug 24 '18 at 15:48
  • Right, the thing is we just can't code for exceptions and error codes in the same place and have both be testable! If they really want error codes, they could wrap new operations in a try/catch, and deliver null and/or error-code on out-of-memory exception -- effectively converting C#'s exception for O-O-M into an error code style. – Erik Eidt Aug 24 '18 at 16:28
  • 1
    The throw-a-exception code could be even further shortened to p = new Object() ?? throw new NullReferenceException(); which would perhaps make it even more apparent candidate for optimization? – wondra Aug 30 '18 at 13:14
  • 1
    There are certain domains where dead code is considered such a huge liability that it is strictly forbidden, FWIW. – RubberDuck Aug 30 '18 at 22:17
8

Perhaps you can dodge the issue and simply let him know that there is a newer c# syntax that is easier (and does the null check for you!)

Instead of

Person p = new Person();
if (p != null)
{
    p.Name = "John Smith";
}

Write

var p = new Person
{
    Name = "John Smith"
};
  • 2
    While that solves the example code, it isn't automatically applicable to every similar case. Substitute the if block's body with var foo = p.DoSomething() and you'll see your answer doesn't apply anymore. – Flater Aug 24 '18 at 15:11
  • That's a good suggestion and could help clean up some cases I'm dealing with. Unfortunately my example was a simplified case of a problem that has many different flavors, so this wouldn't eliminate the problem as a whole. Couldn't hurt though! – Jason Tyler Aug 24 '18 at 15:42
  • 1
    @Flater The pranoid person in question could then go with: var foo = p?.DoSomething(); – Eternal21 Aug 28 '18 at 19:58
4

It is a reasonable request but it sounds like this has already become a struggle between you and him. You may have already pointed it out to him, he is reluctant to change it and now you plan to escalate the matter. Then you may "win".

It may be smoother to just send him the link Robert Harvey posted and say "I sent you a link about C# object construction you may find interesting" and leave it there. It is not exactly harmful what he does (just pointless) and easy to clean op later. I am saying: choose your battles. I have been in this situation more than once and in retrospect, often it just wasn't worth the struggle. You all too easily come to hate each other over virtually nothing. Yes, you are right but while it is just you and him, you may want to preserve some common ground.

  • Thanks for the insight. Your points are well taken. I agree that battles must be chosen wisely. I've tried the "Hey, check out this email" thing, but it usually is ignored. I'm being a stickler about it because this type of "just in case" code is becoming more pervasive in our code base. It makes everything more nondeterministic, so it's harder for me to work in. Maybe I should try and convey that overall point rather than focus on a specific coding pattern? – Jason Tyler Aug 24 '18 at 15:39
1

Since your technical question has already been answered (the null check is pointless), I'd like to focus on a different aspect of your question - how to deal with such a situation in general: a coworker who doesn't know some basic facts about the tools you all use, and thus uses the tools in a less than optimal manner.

In some cases, it may not affect you much. If they get their job done and don't interfere with yours, you can just ignore the issue.

In other cases (e.g. yours), they do interfere with your work, at least to some extent. What to do about it?

I think it depends on a lot of details. For example, how long have you and your coworker been with the company, how does the company generally deal with such issues, how much does the issue affect / annoy you, how well do you get along with the coworker, how important is a harmonious relationship with your coworker to you, to what extent could bringing up or escalating the issue affect this relationship, etc.

My personal take might be this: I think engineers should know their tools, and software engineers should know their programming languages. Of course, no one knows every detail, but that constructors never return null is a very basic idea - everyone should know it. When I need to work on some code that contains such pointless null checks, I'd simply remove them. When a colleague asks me why I removed them, I'd explain why they are pointless. (If necessary, I'd also explain why code that is entirely unnecessary should be removed.) In the long run, this would lead to code that is free of these pointless null checks.

This may sound somewhat arrogant, and it may not be the right approach for everyone. I've been told that I'm patient and good at explaining things. Maybe that's why I tend to get away with this approach without coming across as a jerk. :-)

You mention 'a formal request that this practice be stopped'. I've never worked in a company which had such detailed formal rules, but here are a few thoughts anyway. Again, I think your best course of action depends on the details. If filing such a request would make your coworker look bad in the eyes of colleagues or managers, I'd probably not do it. On the other hand, if such requests are generally seen as a welcome improvement and others won't blame your coworker for not following this rule earlier, go ahead. No one will be harmed, everyone will be better off.


But if I couldn't find a good way to deal with the issue, I might resort to sarcasm and add code like this everywhere:

int c = a + b;
if (c == a + b)
{
    // next steps
}
else 
{
    throw new ArithmeticException();
}

It's just as pointless as a null check after a constructor call, but even more obvious. And of course, I wouldn't really do this. Unless I'm so annoyed with my coworkers that I'm planning to quit anyway. ;-)

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.