Software Development

Join Null Check with Assignment

2017-07-16-join-null-check-with-assignment

I recently wrote some code and asked ReSharper to add a null check for me, which it did. Then it suggested that I could simplify the null check by joining it to the assignment.

Intrigued, I let it.

The code went from this:

public void SetMessage(string message)
{
    if (message == null) throw new ArgumentNullException(nameof(message));
    Message = message;
}

To this:

public void SetMessage(string message)
{
    Message = message ?? throw new ArgumentNullException(nameof(message));
}

So, I assign message to the property Message unless it is null in which case I throw the exception. This is a new feature in C# 7 called a “throw expression”.

At first glance, I thought it would still assign null to Message before throwing the exception, but that’s not what the code looks like underneath.

I got out my trusty dotPeek to see what it actually compiled to. (Don’t worry, I’m not going to show you IL, just what the C# looks like without the syntactic sugar). The result was this:

public void SetMessage(string message)
{
  string str = message;
  if (str == null)
    throw new ArgumentNullException("message");
  this.Message = str;
}

Excellent, it is still doing the null check in advance. So the semantics of what I wrote have not changed. That’s great. I learned something new today.

But…

ReShaper also suggested it in an overloaded version of that function that takes two parameters. And the result was not semantically equivalent. So, be careful. Here’s what happened there. I started with this:

public void SetMessage(string message, string transitionMessage)
{
    if (message == null) throw new ArgumentNullException(nameof(message));
    if (transitionMessage == null) throw new ArgumentNullException(nameof(transitionMessage));

    Message = message;
    TransitionMessage = transitionMessage;
}

Let ReSharper refactor to this:

public void SetMessage(string message, string transitionMessage)
{
    Message = message ?? throw new ArgumentNullException(nameof(message));
    TransitionMessage = transitionMessage ?? throw new ArgumentNullException(nameof(transitionMessage));
}

And, I’m beginning to get a little apprehensive at this point because I think I see a problem. In fact, when I look at it in dotPeek, I can see exactly what the issue is. Here’s the same code with the syntactic sugar removed:

public void SetMessage(string message, string transitionMessage)
{
  string str1 = message;
  if (str1 == null)
    throw new ArgumentNullException("message");
  this.Message = str1;
  string str2 = transitionMessage;
  if (str2 == null)
    throw new ArgumentNullException("transitionMessage");
  this.TransitionMessage = str2;
}

It does the first null check, then assigns to the Message property. Then it does the second null check… And that’s not what I want at all. This method should be an all or nothing proposition. Either both properties are set, or neither are changed and this isn’t the case any more.

Caveat Programmator, as they say in Latin.

7 thoughts on “Join Null Check with Assignment

  1. I’m sorry, I will explain.
    If I understand you correctly you want this instructions

    string str2 = transitionMessage;
    if (str2 == null)
    throw new ArgumentNullException("transitionMessage");

    be followed by

    this.Message = str1;

    That means compiler should change the order of instructions separated by ‘;’
    So I’m surprised you assumed it is possible at all.
    Lets take this program:

    Message = message ?? throw new ArgumentNullException(nameof(argument1));
    TransitionMessage = Message ?? throw new InvalidOperationException();

    As we see the result of second “if” in this case is not determined before executing “Message =..” and it is not possible to execute it before that

    1. I don’t want that at all. I want ReSharper to maintain semantic equivalence in its refactoring. If it can’t do it, it should not suggest it.

      My expectation was based on the fact that R# generally does an excellent job of maintaining the semantics when applying its refactoring, not on how the compiler should interpret the code.

      In this case R# did not do as expected (i.e. maintain semantics), and I was merely pointing out that it did not do what many R# users would expect so they didn’t end up with buggy code.

    1. There is no return statement in the examples. If you meant use the throw with the if statement then, as with many things, it depends.

      If you only have one parameter and your assignment is on the first line, then feel free to join the null check with the assignment. It is semantically the same as before and you may find the structure easier.

      If you have multiple parameters that need a null check, or you need to do additional checks before the assignment, then do all that up-front as if (x == null) throw ArgumentNullException() statements. If you are back

      There is no definite rule to say you should always do it one way or another. Do what makes sense to you, so long as you maintain the functionality that you want. ReSharper frequently makes suggestions on refactorings that I don’t accept because I feel that the way I wrote it is easier to understand.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s