Code suggestions by Resharper making code less readable?

Question

While trying to get to all green, i got the following suggestion by Resharper.

Original code:

    static public string ToNonNullString(this XmlAttribute attr)
    {
        if (attr != null)
            return attr.Value;
        else
            return string.Empty;
    }

Suggestion: remove redundant 'else' resulting in following:

    static public string ToNonNullString(this XmlAttribute attr)
    {
        if (attr != null)
            return attr.Value;
        return string.Empty;
    }

To me, the suggested version seems less readable than the original. Does Resharper suggestion reflect the definition of good maintainable code?

Answer 1

I agree that the first version of your code is more readable.

I've found Resharper suggestions in these cases to not always be helpful, although sometimes it can clean things up. That's why I've configured Resharper to show the change as a "Hint" rather than "Suggestion". This causes the green underline to be less visible and it won't be highlighted in the right sidebar.

Answer 2

Technically Resharper is correct in that the "else" is unnecessary, I prefer the former version though as the intent is more obvious.

Having said that, I'd rather go with:

return attr != null ? attr.Value : string.Empty;

Answer 3

Ah, code aesthetics. Holy war time. (ducks)

I'd go with either a ?: expression:

return attr != null ? attr.Value : String.Empty

or invert the if and remove the line break to produce a guard clause:

if (attr == null) return String.Empty;

return attr.Value;

Answer 4

Your original code is much more readable and understandable - at a glance, you can see exactly the condition which causes string.Empty to be returned. Without the else, you have to see that there is a return in the if block before that.

Just remember, you're a human and inherently smarter than the machine. If it tells you something is better and you don't agree, then don't listen to it.

Answer 5

I've noticed the same thing about ReSharper so I do appreciate its ability to turn some items off or downgrade their warning level. I also am perplexed by this suggestion:

SomeClass varName = new SomeClass();

has a suggested change to:

var varName = new SomeClass();

Yes, I know that I don't need the initial declaration type but it feels odd to suggest that the var form is somehow better than the other. Is anyone familiar with the rationale behind the suggestion or do you agree with me that this one is odd as well?

Answer 6

Classic example of the exceptions that creep into everything when you use a small sample size. Refactoring a huge if-elseif-else block into a guard clause layout makes code far more readable, but, as you say, if you apply the same rules to a single if-else it's not as helpful. I'd go so far as to say it was a (slight) lack of foresight by the resharper devs not to skip over very small blocks such as this, but it's harmless enough.

Answer 7

I think the new version is much better if you invert the if

static public string ToNonNullString(this XmlAttribute attr)
{
    if (attr == null)
        return string.Empty;

    return attr.Value;
}

Because your original version is too symmetric, while null-case is a special case.

New version is more readable in the terms of "what does it return most of the time?".

Answer 8

Some colleagues of mine started using Resharper from that moment on pages they edited were terrible in layout and readability. I'm happy to say it didn't survive, nobody here is using it anymore.

About the statement at hand i agree with Jeffrey Hantin, the inline-if is very nice for such a type of statement and Whatsit's solution is highly accaptable to. With some exceptions i (personaly) say methods/functions should have only 1 return statement.

Also if you (almost) always implement the else with your if (even if it's nothing else but a comment-line stating you do nothing in with the else-statement) it forces you to think about that situation, more often than not it can prevent mistakes.

Both remarks should be used as a 'think about' not like a rule, just like most of this kind of issues, always use your brain :) most errors occur when you don't.

In conclusion: Say NO to Resharper! (yes, I truely dislike Resharper, sorry.)

Answer 9

Being a noob at C# and more used to C and Java I still can't get used to the placement of angle brackets in C# .NET and VS. Putting all that aside, I agree with Andrey in that inverting the 'if' is even more readable. On the other hand I personally find that omitting the 'else' reduces readability (slightly). I would go with this personally:

static public string ToNonNullString(this XmlAttribute attr)
{    
    if (attr == null)
        return string.Empty;
    else
        return attr.Value;
}

Answer 10

If you do not like the way ReSharper suggests something, just disable the specific suggestion (slash warning slash hint). The same goes for coding style, which I think is quite highly configurable. Claiming ReSharper to be unusable (quoting "I'm happy to say it didn't survive, nobody here is using it anymore") just because you do not take 5 minutes to get know how to configure it is just plain stupid.

Of course you should not let some tool dictate some part of your coding style, and ReSharper is NOT doing that if you tell it not to. It's that simple.

Answer 11

The only thing I'd add would have to to with the length of the expressions involved. Personally, I like the compactness of ternary expressions, but turning something like

if (testDateTime > BaseDateTime)
    return item.TransactionDate <= testDateTime && item.TransactionDate >= BaseDateTime;

return item.TransactionDate >= testDateTime && item.TransactionDate <= BaseDateTime;

into something like

return testDateTime > BaseDateTime ? item.TransactionDate <= testDateTime && item.TransactionDate >= BaseDateTime : item.TransactionDate >= testDateTime && item.TransactionDate <= BaseDateTime;

doesn't really seem helpful to me.

Answer 12

My coding standard is allays use brackets (even if there is only one instruction after if command)
This is requires a bit effort (more typing) but I so often become convinced that it is very worth it!

One of most common bug (and paradoxically difficult to find) is adding additional instruction after if statement and forgetting adding brackets...

So I like what Resharper proposed. Especially when having nested if-statements:

Assume we have this code:

   if (condition1)  {
      instruction1;
   }
   else {
       if (condition2) {
           instruction2;
       }
   }

It can be changed to look like this:

   if (condition1)  {
      instruction1;
   }

   // ELSE
   if (condition2) {
      instruction2;
   }       

And this is much more readable to me then before.
(It would be also more visible when you have more than 2-level nested statements)

Answer 13

Its always debatable when it comes to best practices and coding standards. One of the reason for this is they cannot be enforced very easily using an IDE like Visual Studio. There are tools available like FxCop and StyleCop which can be used to analyse the code for standards. FxCop is used for compiled code analysis and StyleCop is used for source code analysis.

You can configure StyleCop to a minute level as to which formatting you would like to apply to the code. There is an add-in called StyleCop for Resharper which gives suggessions right inside Visual Studio. I had a detailed blog post about the same at http://nileshgule.blogspot.com/2010/10/refactoring-clean-code-using-resharper.html