belav / csharpier

CSharpier is an opinionated code formatter for c#.
https://csharpier.com
MIT License
1.41k stars 99 forks source link

Potential new ternary formatting #1254

Closed belav closed 5 months ago

belav commented 6 months ago

The current formatting of nested ternary operators

var languageCode = something.IsNotBlank()
    ? something
    : otherThing.IsNotBlank()
        ? otherThing
        : thingThing.IsNotBlank()
            ? thingThing
            : "enUs";

If that were to change to this, then I believe it becomes a lot more readable.

var languageCode = 
    something.IsNotBlank() ? something
    : otherThing.IsNotBlank() ? otherThing
    : thingThing.IsNotBlank() ? thingThing
    : "enUs";

Would also need to consider what to do if someone creates a statement like this

var x = firstCondition
    ? secondCondition
        ? firstValue
        : secondValue
    : thirdCondition
        ? thirdValue
        : fourthValue;
belav commented 6 months ago

For way too much discussion - https://github.com/prettier/prettier/issues/5814

JS devs seem to be bigger fans of ternarys.

asik commented 6 months ago

The nesting should follow the semantic nesting of the expression. Something like:

condition1 ? value1 :
condition2 ? value2 :
defaultValue;

Is the same as an if - else if - else structure: there's no nesting going on semantically and there shouldn't be visually. If lines are too long they can be broken down like:

condition1 ?
     value1 :
condition2 ?
    value2 :
defaultValue;

The indentation then reflects the condition - result relationship which helps readability and is analogous to if-else structures.

You get nesting of conditions when you have more than one ? in a row:

condition1 ?
    condition 2 ? value2:
    etc.

The prettier discussion seemed centered around this proposal:

condition1
? result1
: condition2
? result2

Which frankly I don't think anyone would find readable as the indentation does nothing to highlight the relationship between conditions and values.

belav commented 5 months ago

For those that do not like this change, why?

Compare the proposed change

var parameterType =
    property.PropertyType == typeof(string) ? "string"
    : property.PropertyType == typeof(int) ? "int"
    : property.PropertyType == typeof(int?) ? "int?"
    : property.PropertyType == typeof(DateTimeOffset) ? "DateTimeOffset"
    : property.PropertyType == typeof(DateTimeOffset?) ? "DateTimeOffset?"
    : property.PropertyType == typeof(DateTime) ? "DateTime"
    : property.PropertyType == typeof(DateTime?) ? "DateTime?"
    : property.PropertyType == typeof(decimal) ? "decimal"
    : property.PropertyType == typeof(decimal?) ? "decimal?"
    : property.PropertyType == typeof(Guid) ? "Guid"
    : property.PropertyType == typeof(Guid?) ? "Guid?"
    : property.PropertyType == typeof(bool) ? "bool"
    : property.PropertyType == typeof(bool?) ? "bool?"
    : property.PropertyType == typeof(byte[]) ? "byte[]"
    : property.PropertyType == typeof(float) ? "float"
    : property.PropertyType.Name;

To the current way csharpier formats it

var parameterType =
    property.PropertyType == typeof(string)
        ? "string"
        : property.PropertyType == typeof(int)
            ? "int"
            : property.PropertyType == typeof(int?)
                ? "int?"
                : property.PropertyType == typeof(DateTimeOffset)
                    ? "DateTimeOffset"
                    : property.PropertyType == typeof(DateTimeOffset?)
                        ? "DateTimeOffset?"
                        : property.PropertyType == typeof(DateTime)
                            ? "DateTime"
                            : property.PropertyType == typeof(DateTime?)
                                ? "DateTime?"
                                : property.PropertyType == typeof(decimal)
                                    ? "decimal"
                                    : property.PropertyType == typeof(decimal?)
                                        ? "decimal?"
                                        : property.PropertyType == typeof(Guid)
                                            ? "Guid"
                                            : property.PropertyType == typeof(Guid?)
                                                ? "Guid?"
                                                : property.PropertyType == typeof(bool)
                                                    ? "bool"
                                                    : property.PropertyType == typeof(bool?)
                                                        ? "bool?"
                                                        : property.PropertyType == typeof(byte[])
                                                            ? "byte[]"
                                                            : property.PropertyType == typeof(float)
                                                                ? "float"
                                                                : property.PropertyType.Name;

And compare that to the same logic moved into a switch expression

var parameterType = property.PropertyType switch
{
    Type t when t == typeof(string) => "string",
    Type t when t == typeof(int) => "int",
    Type t when t == typeof(int?) => "int?",
    Type t when t == typeof(DateTimeOffset) => "DateTimeOffset",
    Type t when t == typeof(DateTimeOffset?) => "DateTimeOffset?",
    Type t when t == typeof(DateTime) => "DateTime",
    Type t when t == typeof(DateTime?) => "DateTime?",
    Type t when t == typeof(decimal) => "decimal",
    Type t when t == typeof(decimal?) => "decimal?",
    Type t when t == typeof(Guid) => "Guid",
    Type t when t == typeof(Guid?) => "Guid?",
    Type t when t == typeof(bool) => "bool",
    Type t when t == typeof(bool?) => "bool?",
    Type t when t == typeof(byte[]) => "byte[]",
    Type t when t == typeof(float) => "float",
    _ => property.PropertyType.Name
};

// or alternatively
var parameterType = property.PropertyType switch
{
    { } when property.PropertyType == typeof(string) => "string",
    { } when property.PropertyType == typeof(int) => "int",
    { } when property.PropertyType == typeof(int?) => "int?",
    { } when property.PropertyType == typeof(DateTimeOffset) => "DateTimeOffset",
    { } when property.PropertyType == typeof(DateTimeOffset?) => "DateTimeOffset?",
    { } when property.PropertyType == typeof(DateTime) => "DateTime",
    { } when property.PropertyType == typeof(DateTime?) => "DateTime?",
    { } when property.PropertyType == typeof(decimal) => "decimal",
    { } when property.PropertyType == typeof(decimal?) => "decimal?",
    { } when property.PropertyType == typeof(Guid) => "Guid",
    { } when property.PropertyType == typeof(Guid?) => "Guid?",
    { } when property.PropertyType == typeof(bool) => "bool",
    { } when property.PropertyType == typeof(bool?) => "bool?",
    { } when property.PropertyType == typeof(byte[]) => "byte[]",
    { } when property.PropertyType == typeof(float) => "float",
    _ => property.PropertyType.Name
};

I don't see any reason that a chained ternary needs extra indentation.

Nested ternarys will continue to be indented the way they were.


return firstCondition
    ? secondCondition
        ? firstValue
        : secondValue
    : thirdCondition
        ? thirdValue
        : fourthValue;
shocklateboy92 commented 5 months ago

For those that do not like this change, why?

I think you didn't emphasize the difference between a chained ternary and a nested ternary.

How often do you find chained ternaries in the wild? Is there a reason to use them instead of switch statements?

loraderon commented 5 months ago

I don't use that many. It looks way better when there is two or three which I think is the most that's acceptable

belav commented 5 months ago

How often do you find chained ternaries in the wild? Is there a reason to use them instead of switch statements?

We have them scattered around in our codebase at work. CSharpier is responsible for formatting what it finds regardless of opinions on if switch expressions should be used instead chained ternarys. Switch expressions also don't operate on conditions, although you can use pattern matching to hack around that. I prefer the conciseness of ternarys - https://github.com/dotnet/csharplang/discussions/3350#discussioncomment-9349085