Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

What's the difference between ReSharper `MergeSequentialChecks` and `MergeSequentialChecksWhenPossible`?

I'm trying to figure out what's the difference between these two rules?

  • MergeSequentialChecks
  • MergeSequentialChecksWhenPossible

The documentation doesn't say anything about the second one. https://www.jetbrains.com/help/resharper/2016.1/MergeSequentialChecks.html

And it's not quire clear for me what does it mean WhenPossible?

If ReSharper suggests to apply the first rule and merge my sequential checks, then it IS possible indeed. How it could be not possible?

Here is a code example to check.

public class Person
{
    public string Name { get; set; }
    public IList<Person> Descendants { get; set; }
}

public static class TestReSharper
{
    // Here `MergeSequentialChecks` rule is triggered for both `&&` operands.
    public static bool MergeSequentialChecks(Person person)
    {
        return person != null && person.Descendants != null && person.Descendants.FirstOrDefault() != null;
    }

    // Here `MergeSequentialChecksWhenPossible` rule is triggered.
    public static bool MergeSequentialChecksWhenPossible1(Person person)
    {
        return person != null && person.Descendants.Any();
    }

    // Here `MergeSequentialChecksWhenPossible` rule is triggered.
    public static bool MergeSequentialChecksWhenPossible2(Person person)
    {
        return person.Descendants != null && person.Descendants.Any();
    }
}
like image 549
gordey4doronin Avatar asked Oct 19 '16 15:10

gordey4doronin


1 Answers

The idea behind code inspections with "(when possible)" label is simple: we decided not to suggest possible code transformation with the default R# settings, since produced code may result in reduced readability or became harder to understand. This decision on what to suggest or not is done via ad-hoc heuristics.

When we first implemented C# 6.0 related code suggestions and transformations and reviewed their results in big solutions available for us, we decided that nearly ~2/3 of occurrences of inspections like "Merge sequential checks" / "Use null propagation" should not suggest doing the code transformations. For example, we think that in the case of code like this:

if (node != null && node.IsValid()) { ... }

...there are no real benefits of using ?. operator and introduction of "lifted" operator==(bool, bool) operator over value of bool? type:

if (node?.IsValid() == true) { ... }

Plus different devs have different views on how to check value of type bool? for being true (some prefer to have ?? false instead, but that would make the resulting code even?.more ?? questionable).

So, the in the case above the merge of sequential checks is definitely "possible", but we are not gonna recommend it with the default R# settings (proper default settings is a huge responsibility), leaving user with the ability to see all cases by enabling the "(when possible)"-version of the same code inspection. As far I can see, currently we only check for not producing lifted boolean checks in "Merge sequential checks" inspection, other inspections have more heuristics, for example:

if (stringBuilder != null) { // "Use null propagation"
  stringBuilder.Append("hello");
}

Code above worth suggesting to use the conditional invocation operator:

stringBuilder?.Append("hello");

While using the conditional invocation twice or more times sequentially is questionable...

if (stringBuilder != null) { // no suggestion
  stringBuilder.Append("hello");
  stringBuilder.Append("world");
  stringBuilder.Append("!!!");
}

p.s. Context actions "Merge sequential checks" / "To null propagation" are always enabled when transformation is possible, despite the state of code inspections and their severities.

like image 134
controlflow Avatar answered Oct 22 '22 19:10

controlflow