Legible or not: C# multiple ternary operators + Th

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

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 2

In C & C++, the described use is idiomatic and the reason for the operator. The benefit of the ternary conditional vs. a chained if-then-else is that it's an expression with a known type. In C++, you can actually write

foo_t foo = bar_p ? bar
          : qux_p ? qux
          : woz_p ? woz
          : (throw SomeExpression(), foo_t())
          ;

Notice the comma operator, which returns a foo_t that will never be constructed due to the throw.

Answer 3

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 4

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 5

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 6

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);
}