New readability rule to format ternary expression

[Przemyslaw-W]

Hi,

I'd like to propose new readability rule for ternary expressions. Valid options should be:

1. Whole expression in one line:

   int x = booleanExpression ? firstIntValueExpression : secondIntValueExpression;

2. Each subexpression on its own line, with ? and : as starting characters:

   int x = booleanExpression
      ? firstIntValueExpression
      : secondIntValueExpression;

Rationale for placement expressions in separate lines:

• consistency with parameters and statement rules - parameters should be in one line or each in separate lines, statements should be each in its own line.
• each expression in its own line allows to easy grasp where it starts and ends.

Rationale for ? and : placement:

• when these operators are placed at the beginning of line, one can easy see that rest of the line is part of ternary expression.
• when these operators are placed at the end of line and expressions are long, ? and : tokens can go out of view. This makes harder to see that ternary expression is used.

Here is corresponding original SC issue, together with follow up discussion: https://stylecop.codeplex.com/discussions/253622

[AArnott]

:+1: I already follow this rule and ask my team to follow it. I very much agree it helps with readability. And if folks can't fit each sub-expression on a single line, maybe they shouldn't be using the ternary operators.

[sharwell]

:+1: I agree on this one too.

[sharwell]

Moved this one forward.

[vweijsters]

:+1: for me as well

@sharwell I think it would a good idea if you would assign an identifier to an accepted proposal (preferably in the title of the issue).

[otac0n]

p == "Cat" ? "Animals" :
p == "Bat" ? "Equipment" :
p == "Fat" ? "Nutrition" : "None"

Should be allowed, since each line is a logical unit.

[sharwell]

It looks like you are trying to treat the ternary operator like a match operator from functional programming. A better form for the expression, and one that is compatible with this rule, is using either a switch statement or a sequence of if/else statements.

[otac0n]

So you are saying that this:

var x = p == "Cat" ? "Animals" :
        p == "Bat" ? "Equipment" :
        p == "Fat" ? "Nutrition" : "None";

MUST be written as follows, in order to be compliant?:

string x;
if (p == "Cat")
{
    x = "Animals";
}
else if (p == "Bat")
{
    x = "Equipment";
}
else if (p == "Fat")
{
    x = "Nutrition";
}
else
{
    x = "None";
}

Are you serious? Do you not see how much harder that is to read, write, and maintain? StyleCop is supposed to improve readability, not make it worse. Consistency is a laudable goal, but never at the price of readability.

[AArnott]

I agree with @otac0n for the multiple expression case. Perhaps we can have the originally proposed format encouraged for single expressions and @otac0n's format for multi-expressions. Unless parentheses are involved, in which case, I suggest stylecop just let it be however the developer wrote it.

[otac0n]

:+1:

On Tue, Aug 4, 2015, 4:48 PM Andrew Arnott notifications@github.com wrote:

I agree with @otac0n https://github.com/otac0n for the multiple expression case. Perhaps we can have the originally proposed format encouraged for single expressions and @otac0n https://github.com/otac0n's format for multi-expressions. Unless parentheses are involved, in which case, I suggest stylecop just let it be however the developer wrote it.

— Reply to this email directly or view it on GitHub https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/651#issuecomment-127798346 .

[otac0n]

I should elaborate to say that what @AArnott described is exactly how I've been formatting my code for a couple of years. I think it turns out very readable. (e.g. single conditional vs multiple conditionals)

[sharwell]

I think it turns out very readable.

I find the second example is good motivation for this rule without the exclusion. The following is more lines of code, but readability is dramatically improved.

if (this.sourceIdentifier == null)
{
  return "create " + this.canonicalName + " => " + this.targetIdentifier;
}
else if (this.targetIdentifier == null)
{
  return "delete " + this.canonicalName + " (was " + this.sourceIdentifier + ")";
}
else
{
  return "update " + this.canonicalName + " => " + this.targetIdentifier + " (was " + this.sourceIdentifier + ")";
}

[otac0n]

Yeah, if I had been aligning horizontally, readability could have been improved:

return this.sourceIdentifier == null ? "create " + this.canonicalName + " => " + this.targetIdentifier :
       this.targetIdentifier == null ? "delete " + this.canonicalName + " (was " + this.sourceIdentifier + ")" :
                                       "update " + this.canonicalName + " => " + this.targetIdentifier + " (was " + this.sourceIdentifier + ")";

And with C# 6 it's even better:

return this.sourceIdentifier == null ? $"create {this.canonicalName} => {this.targetIdentifier}" :
       this.targetIdentifier == null ? $"delete {this.canonicalName} (was {this.sourceIdentifier})" :
                                       $"update {this.canonicalName} => {this.targetIdentifier} (was {this.sourceIdentifier})";

I do take your point, though.

Is there a simple rule we can use here that won't reduce the readability of existing code? Perhaps, just keep the original proposal for single conditionals and ignore the rest?

[sharwell]

Is there a simple rule we can use here that won't reduce the readability of existing code?

So far I haven't seen cases that would be negatively impacted by the original rule proposal, but I have seen cases that would be positively impacted. Once the rule is in the wild (in a beta) we'll have a much better idea of whether or not exclusions are actually needed.

[AArnott]

Just a thought: multiple check cases can often be rewritten as switch statements (not always, of course, since switch expressions must be one of very few primitive types).

[GregReddick]

It should be noted that they are talking about adding a match operator to C# version 7 as part of the standard C# syntax. https://github.com/dotnet/roslyn/issues/206 and https://github.com/dotnet/roslyn/blob/future/docs/features/patterns.md

[julealgon]

Folks, was this abandoned? It sounded like a fair proposal, but for some odd reason discussions abruptly stopped 3 years ago?

[SpaceKatt]

Any update on this issue? I want to enforce this rule.

[sharwell]

So you are saying that this:

var x = p == "Cat" ? "Animals" :
        p == "Bat" ? "Equipment" :
        p == "Fat" ? "Nutrition" : "None";

Now that switch expressions are out, we can do this:

var x = p switch
{
  "Cat" => "Animals",
  "Bat" => "Equipment",
  "Fat" => "Nutrition",
  _ => "None",
};

[sharwell]

Marked up for grabs, again using the original proposal.

[AArnott]

I would rather see the original proposal modified to accommodate @otac0n's proposal, which for multiple ternary expressions allows this formatting:

var x = p == "Cat" ? "Animals" :
        p == "Bat" ? "Equipment" :
        p == "Fat" ? "Nutrition" : "None";

If the diagnostic is to recommend switch expressions, we should only recommend that when doing so would be valid C#. I believe the left side of the => has to be a constant or pattern matching expression, which is a subset of what you can put in any expression that could appear in a ternary condition.

[julealgon]

I believe the left side of the => has to be a constant or pattern matching expression, which is a subset of what you can put in any expression that could appear in a ternary condition.

You would be incorrect there I think, in the sense that a pattern matching expression paired with a when allows you to perform arbitrary checks (same as you could with a ternary operator).

[AArnott]

Yes, I suppose you're right. But an automated code fix would have to know how to split an expression into the pattern matching part and the when part. Unless you just set all patterns to true and put the whole expression under when, but that doesn't feel like a good use of switch expressions, IMO.

[sharwell]

@AArnott I lead towards starting with the simple form of the rule and then evaluate it on real-world solutions to see if it needs alteration