Stylecopanalyzers: 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:

c# int x = booleanExpression ? firstIntValueExpression : secondIntValueExpression;

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

c# 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 [email protected] 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?