Merge Sequential Check

Question

There is a class A with Nullable property Prop

public class A {
    int? Prop {get; set;} 
}

...

I have object a which is type A On the conditional:

if (a != null && a.Prop.HasValue) { // <--
   int res = a.Prop;
}

I am getting suggestion "Merge sequential checks"

I don't understand how could I do that. a?.Prop.HasValue will not work

What am I missing here?

Answer 1

Resharper converts your original logic:

if (a != null && a.Prop.HasValue)

into this:

if (a?.Prop != null)

So, the block will execute as long as the expression does not evaluate to null, and it will evaluate to null if either a is null (thanks to ?.) or if a.Prop is null (as normal). In both versions, you're basically saying "do this stuff as long as both a and a.Prop have values" (or really "...don't not have values").

I'm not entirely sure I like these refactorings, though. Indeed, I can see in this other question that JetBrains themselves don't necessarily think these are good refactorings, because they make the code harder to understand.

In fact, I think probably specifically because these forms are harder to understand, this "Merge Sequential Checks" refactoring is actually broken in some cases, since at least end of April 2017 (also here), and still currently as of end of Feb 2018.

Beware when using the refactoring when your expression contains both an || operator, and either of the Any or All LINQ methods (and other cases, too, like a function call returning a bool).

Given:

var array = new[] { 1, 2, 3, 4, 5 };

Resharper will convert this:

if (array == null || !array.Any())

into this:

if (!array?.Any() != true) // WRONG

The original was clearly "do this stuff if array is null, or if array is empty".

But what does the second version say? It's so hard to read you can barely tell if it's right or wrong.

!= true effectively means "do this if the expression was null or false". Ok, so we'll do it if array is null, that's good so far. But, if array is not null, then what?

Well, we want to do the stuff if the array is empty. If the array is empty, then array?.Any() will be false.

Then we have !false, which is true.

And then we have true != true which is false.

And so we have "if the array is empty ... don't execute the block", which is the opposite of what we wanted.

So Resharper has gotten the logic backwards, and the block will execute if there is a value, rather than if there is not a value.

Even without this flaw, the code is just plain hard to read, so I feel like this refactoring should generally be avoided.