Legible or not: C# multiple ternary operators + Throw if unmatched.

Question

Do you find the following C# code legible?

private bool CanExecuteAdd(string parameter) {
    return
        this.Script == null ? false
        : parameter == "Step" ? true
        : parameter == "Element" ? this.ElementSelectedInLibrary != null && this.SelectedStep != null
        : parameter == "Choice" ? this.SelectedElement != null
        : parameter == "Jump" ? this.SelectedStep != null
        : parameter == "Conditional jump" ? false
        : false.Throw("Unknown Add parameter {0} in XAML.".F(parameter));
}

where Throw is defined as:

public static T Throw<T>(this T ignored, string message) {
    throw new Exception(message);
}

I know that's not idiomatic C#. However, would you be able at understand it at first or second glance? Or did I stray too far?

Answer 1

I've used this sort of code in Java a fair amount. I don't like the false.Throw bit, but having multiple conditionals like this (particularly formatted this way) is fine in my view.

It's slightly strange the very first time you see it, but after that it's just a handy pattern to know about.

One alternative to using false.Throw here would be something like this:

bool? ret = this.Script == null ? false
    : parameter == "Step" ? true
    : parameter == "Element" ? this.ElementSelectedInLibrary != null && this.SelectedStep != null
    : parameter == "Choice" ? this.SelectedElement != null
    : parameter == "Jump" ? this.SelectedStep != null
    : parameter == "Conditional jump" ? false
    : null;

if (ret == null)
{
    throw new ArgumentException(
       string.Format("Unknown Add parameter {0} in XAML.", parameter);
}
return ret.Value;

EDIT: Actually, in this case I wouldn't use either if/else or this pattern... I'd use switch/case. This can be very compact if you want:

if (this.Script == null)
{
    return false;
}
switch (parameter)
{
    case "Step": return true;
    case "Element": return this.ElementSelectedInLibrary != null && this.SelectedStep != null;
    case "Choice": return this.SelectedElement != null;
    case "Jump": return this.SelectedStep != null;
    default: throw new ArgumentException(
        string.Format("Unknown Add parameter {0} in XAML.", parameter);
}

Answer 2

Being non-idiomatic means that you're forcing the reader to spend time thinking about whether or not what they're reading means what they think it means.

So being legible doesn't buy the sophisticated (namely, suspicious) reader very much. This strikes me as a case of being clever for the sake of being clever.

Is there any reason not to use a switch or an else if construct here?

Answer 3

I really don't like this code. It took me more than about 15 seconds to understand, so I gave up.

An if/then would be preferable.

Answer 4

I've actually never seen the ternary operator pushed out so far. However, I understood where you were going. Also, I agree with Jon, I don't like the false.Throw part.

Answer 5

I vote for non-legible.

Although the syntax is correct, it's somewhat convoluted and since it's not, dare I say, "traditional", many developers will have to waste time trying to ensure they understand what they're reading. Not an ideal situation.

Readability is definitely one key ingredient to good coding, and I would say your sample isn't immediately readable for most devs.

Answer 6

Why not use a nullable type bool?

private bool? CanExecuteAdd(string parameter) {
return
    this.Script == null ? false
    : parameter == "Step" ? true
    : parameter == "Element" ? this.ElementSelectedInLibrary != null && this.SelectedStep != null
    : parameter == "Choice" ? this.SelectedElement != null
    : parameter == "Jump" ? this.SelectedStep != null
    : parameter == "Conditional jump" ? false
    : null;

}

Answer 7

At first I was horrified, but actually I can't think of a way to write this much clearer in C# - I was trying to think of something where you had an array of Funcs mapped to results, but it got even uglier.

Even though parsing through the actual conditionals is rough, it's at least easy to grok the intent, though I'd prefer to use a switch block and handle everything else as a special case.

Answer 8

Convert your nested ternary into a switch. Never coerce one control structure into doing poorly or illegibly what a built-in structure will do perfectly, especially if there's no obvious benefit.

Answer 9

I like the conditional operator, and use it a lot.

This example is a little confusing, because the nature of the operator is not clear from the layout and usage.

At the very least I like to make the choice and alternatives clear by using this formatting:

choice
  ? true-case
  : false-case

But if we apply that to your code it reveals the lack of clarity in using the construct this way:

return
    this.Script == null 
                ? false 
                : parameter == "Step" 
                    ? true
                    : parameter == "Element" 
                        ? this.ElementSelectedInLibrary != null && this.SelectedStep != null
                        : parameter == "Choice" 
                            ? this.SelectedElement != null
                            : parameter == "Jump" 
                                ? this.SelectedStep != null
                                : parameter == "Conditional jump" 
                                        ? false
                                        : false.Throw("Unknown Add parameter {0} in XAML.".F(parameter));

This feels to me like we're trying to use the conditional operator like a switch statement, where a switch statement, or better yet a design pattern like the Command Pattern would be much clearer.

Answer 10

Why not use a switch? I think it's way more readable.

private bool CanExecuteAdd(string parameter) {
    if (Script == null)
        return false;

    switch (parameter) {
        case "Step":
            return true;
        case "Element":
            return ElementSelectedInLibrary != null && SelectedStep != null;
        case "Choice":
            return SelectedElement != null;
        case "Jump":
            return SelectedStep != null;
        case "Conditional jump":
            return false;
        default:
            throw new Exception(string.Format("Unknown Add parameter {0} in XAML.", parameter));
    }
}

Answer 11

Too hard to read, take care of exceptions first.

Handle each case in it's own if, then you can have more complex conditions.

This is one of the few times, this many separate returns in a method would be acceptable

private bool CanExecuteAdd(string parameter) 
{       
    if (this.Script == null)
        return false;

    if (parameter.NotIn([] {"Step", "Element", "Choice", "Jump", "Conditional jump"})
        throw new Exception("Unknown Add parameter {0} in XAML.".F(parameter));

    if (parameter == "Step") 
        return true;

    if (parameter == "Element")
        this.ElementSelectedInLibrary != null && this.SelectedStep != null;

        // etc, etc
}

Oh, and the .NotIn is an extension method, the opposite of this, I would imagine (can't say this is quite exact to what is needed)

public static bool In<T>(this T obj, IEnumerable<T> arr)
{
    return arr.Contains(obj);
}

Answer 12

It looks fine to me, but I'd alter your Throw method to:

static TObject Throw<TObject, TException>(this TObject ignored, TException exception)
{
   throw exception;
}

That allows you to specify the kind of Exception thrown.

Answer 13

Unfortunately the ternary operator (?:) isn't a common idiom in the C languages--I've met many C, C++ and C# developers who have to pause to read it because they aren't familiar with it or don't use it. That does not make it a bad language feature or illegible, however those same developers may call OP's example illegible because it nests a language feature they're uncomfortable with.

I don't find the example illegible--I've seen nested ternary operators many times. However, I do feel that using a switch would be a preferable choice for checking 'parameter' against the strings.

Far more galling to me is that Throw extension method that ignores the 'this' parameter. What would 42.Throw(...) mean? If I were reviewing the code I would call it out as bad design.

Answer 14

My rule of thumb: use expressions for things with no side effects. Use statements for things with one side effect and for control flow.

Throwing is effectively a side effect; it does not compute a value, it alters control flow. You're computing a value, computing, computing, computing, and then boom, side effect. I find code like that confusing and vexing. I say that control flow should be in statements, not the side effect of something that looks like a computation.