How to rewrite complicated lines of C++ code (nested ternary operator)

Question

I've been looking through someone else's code for debugging purposes and found this:

!m_seedsfilter ? good=true : m_seedsfilter==1 ? good=newClusters(Sp) : good=newSeed(Sp);  

What does this mean? Is there an automated tool that will render this into more comprehensible if/else statements? Any tips for dealing with complicated control structures like this?

Edit note: I changed this from "unnecessarily complicated" to "complicated" in the title, since it's a matter of opinion. Thanks for all your answers so far.

Solution 1

The statement as written could be improved if rewritten as follows....

good = m_seedsfilter==0 ? true :
       m_seedsfilter==1 ? newClusters(Sp) :
                          newSeed(Sp);

...but in general you should just become familar with the ternary statement. There is nothing inherently evil about either the code as originally posted, or xanatos' version, or mine. Ternary statements are not evil, they're a basic feature of the language, and once you become familiar with them, you'll note that code like this (as I've posted, not as written in your original post) is actually easier to read than a chain of if-else statements. For example, in this code, you can simply read this statement as follows: "Variable good equals... if m_seedsfilter==0, then true, otherwise, if m_seedsfilter==1, then newClusters(Sp), otherwise, newSeed(Sp)."

Note that my version above avoids three separate assignments to the variable good, and makes it clear that the goal of the statement is to assign a value to good. Also, written this way, it makes it clear that essentially this is a "switch-case" construct, with the default case being newSeed(Sp).

It should probably be noted that my rewrite above is good as long as operator!() for the type of m_seedsfilter is not overridden. If it is, then you'd have to use this to preserve the behavior of your original version...

good = !m_seedsfilter   ? true :
       m_seedsfilter==1 ? newClusters(Sp) :
                          newSeed(Sp);

...and as xanatos' comment below proves, if your newClusters() and newSeed() methods return different types than each other, and if those types are written with carefully-crafted meaningless conversion operators, then you'll have to revert to the original code itself (though hopefully formatted better, as in xanatos' own post) in order to faithfully duplicate the exact same behavior as your original post. But in the real world, nobody's going to do that, so my first version above should be fine.

---

UPDATE, two and a half years after the original post/answer: It's interesting that @TimothyShields and I keep getting upvotes on this from time to time, and Tim's answer seems to consistently track at about 50% of this answer's upvotes, more or less (43 vs 22 as of this update).

I thought I'd add another example of the clarity that the ternary statement can add when used judiciously. The examples below are short snippets from code I was writing for a callstack usage analyzer (a tool that analyzes compiled C code, but the tool itself is written in C#). All three variants accomplish exactly the same objective, at least as far as externally-visible effects go.

1. WITHOUT the ternary operator:

Console.Write(new string(' ', backtraceIndentLevel) + fcnName);
if (fcnInfo.callDepth == 0)
{
   Console.Write(" (leaf function");
}
else if (fcnInfo.callDepth == 1)
{
   Console.Write(" (calls 1 level deeper");
}
else
{
   Console.Write(" (calls " + fcnInfo.callDepth + " levels deeper");
}
Console.WriteLine(", max " + (newStackDepth + fcnInfo.callStackUsage) + " bytes)");

2. WITH the ternary operator, separate calls to Console.Write():

Console.Write(new string(' ', backtraceIndentLevel) + fcnName);
Console.Write((fcnInfo.callDepth == 0) ? (" (leaf function") :
              (fcnInfo.callDepth == 1) ? (" (calls 1 level deeper") :
                                         (" (calls " + fcnInfo.callDepth + " levels deeper"));
Console.WriteLine(", max " + (newStackDepth + fcnInfo.callStackUsage) + " bytes)");

3. WITH the ternary operator, collapsed to a single call to Console.Write():

Console.WriteLine(
   new string(' ', backtraceIndentLevel) + fcnName +
   ((fcnInfo.callDepth == 0) ? (" (leaf function") :
    (fcnInfo.callDepth == 1) ? (" (calls 1 level deeper") :
                               (" (calls " + fcnInfo.callDepth + " levels deeper")) +
   ", max " + (newStackDepth + fcnInfo.callStackUsage) + " bytes)");

One might argue that the difference between the three examples above is trivial, and since it's trivial, why not prefer the simpler (first) variant? It's all about being concise; expressing an idea in "as few words as possible" so that the listener/reader can still remember the beginning of the idea by the time I get to the end of the idea. When I speak to small children, I use simple, short sentences, and as a result it takes more sentences to express an idea. When I speak with adults fluent in my language, I use longer, more complex sentences that express ideas more concisely.

These examples print a single line of text to the standard output. While the operation they perform is simple, it should be easy to imagine them as a subset of a larger sequence. The more concisely I can clearly express subsets of that sequence, the more of that sequence can fit on my editor's screen. Of course I can easily take that effort too far, making it more difficult to comprehend; the goal is to find the "sweet spot" between being comprehensible and concise. I argue that once a programmer becomes familiar with the ternary statement, comprehending code that uses them becomes easier than comprehending code that does not (e.g. 2 and 3 above, vs. 1 above).

The final reason experienced programmers should feel comfortable using ternary statements is to avoid creating unnecessary temporary variables when making method calls. As an example of that, I present a fourth variant of the above examples, with the logic condensed to a single call to Console.WriteLine(); the result is both less comprehensible and less concise:

4. WITHOUT the ternary operator, collapsed to a single call to Console.Write():

string tempStr;
if (fcnInfo.callDepth == 0)
{
   tempStr = " (leaf function";
}
else if (fcnInfo.callDepth == 1)
{
   tempStr = " (calls 1 level deeper";
}
else
{
   tempStr = " (calls " + fcnInfo.callDepth + " levels deeper";
}
Console.WriteLine(new string(' ', backtraceIndentLevel) + fcnName + tempStr +
                  ", max " + (newStackDepth + fcnInfo.callStackUsage) + " bytes)");

Before arguing that "condensing the logic to a single call to Console.WriteLine() is unnecessary," consider that this is merely an example: Imagine calls to some other method, one which takes multiple parameters, all of which require temporaries based on the state of other variables. You could create your own temporaries and make the method call with those temporaries, or you could use the ternary operator and let the compiler create its own (unnamed) temporaries. Again I argue that the ternary operator enables far more concise and comprehensible code than without. But for it to be comprehensible you'll have to drop any preconceived notions you have that the ternary operator is evil.

Solution 2

The equivalent non-evil code is this:

if (m_seedsfilter == 0)
{
    good = true;
}
else if (m_seedsfilter == 1)
{
    good = newClusters(Sp);
}
else
{
    good = newSeed(Sp);
}

Chained ternary operators - that is, the following

condition1 ? A : condition2 ? B : condition3 ? C : D

- are a great way to make your code unreadable.

I'll second @phonetagger's suggestion that you become familiar with ternary operators - so that you can eliminate nested ones when you encounter them.

Solution 3

This is better?

!m_seedsfilter ? good=true 
               : m_seedsfilter==1 ? good=newClusters(Sp) 
                                  : good=newSeed(Sp);  

I'll add that, while it's theoretically possible to simplify this expression (by why? It's so much clear!), the resulting expression wouldn't probably be 100% equivalent in all the possible cases... And showing if two expressions are really equivalent in C++ is a problem very very very very very complex...

The degenerate example I have contrived (http://ideone.com/uLpe0L) (note that it isn't very degenerate... It's only based on a small programming error) is based on considering good a bool, creating two classes UnixDateTime and SmallUnixDateTime, with newClusters() returning a SmallUnixDateTime and newSeed() returning a UnixDateTime. They both should be used to contain a Unix datetime in the format of the number of seconds from 1970-01-01 midnight. SmallUnixDateTime uses an int, while UnixDateTime uses a long long. Both are implicitly convertible to bool (they return if their inner value is != 0, something "classical"), but UnixDateTime is even implicitly convertible to SmallUnixDateTime (this is wrong, because there could be a loss of precision... Here it's the small programming error). On failure of the conversion, a SmallUnixDateTime setted to 0 is returned. In the code of this example there will always be a single conversion: between SmallUnixDateTime to bool or between UnixDateTime to bool...

While in this similar-but-different example:

good = !m_seedsfilter ? true 
                      : m_seedsfilter==1 ? newClusters(Sp) 
                                         : newSeed(Sp);

there are two possible paths: SmallUnixDateTime (newClusters(Sp)) is converted to bool or UnixDateTime (newSeed(Sp))is converted first to SmallUnixDateTime and then to bool. Clearly the two expressions aren't equivalent.

To make it work (or "not work"), newSeed(Sp) returns a value that can't be contained in a SmallUnixTime (std::numeric_limits<int>::max() + 1LL).

Solution 4

To answer your main question, this is an example of a conditional expression:

conditional-expression:
    logical-OR-expression
    logical-OR-expression ? expression : conditional-expression

If the logical-OR-expression evaluates to true, then the result of the expression is the expression following the ?, otherwise it's the expression following the :. For example,

x = y > 0 ? 1 : 0;

will assign 1 to x if y is greater than 0, otherwise it will assign '0'.

You're right to feel queasy about the example because it's badly written. The author is trying to use the ?: operator as a control structure, which it's not meant for.

A better way to write this would be

    good = !m_seedsfilter ? true : 
                            ( m_seedsfilter == 1 ? newClusters(SP) : 
                                                   newSeed(SP) );

If m_seedsfilter equals 0, then good will be set to true. If m_seedsfilter equals 1, then good will be set to the result of newClusters(SP). Otherwise, good will be set to the result of newSeed(SP).

Solution 5

Nested ternary statements make code less readable IMHO. I would use them only when they significantly simplify the rest of the code. The quoted code could be re-written like this:

good = !m_seedsfilter ? true : m_seedsfilter==1 ? newClusters(Sp) : newSeed(Sp); 

or like this:

if(!m_seedsfilter)
    good = true;
else if(m_seedsfilter==1)
    good = newClusters(Sp);
else
    good = newSeed(Sp);

The 1st choice is more succinct, but less readable for a novice and less debugable.

View more solutions

Comments

[Admin]

There is no need to re-write it. It is very clear and simple. What you need is to learn about ternary operator and operator precedence in general.

[xanatos]

I'll say that even Chinese is very clear and simple for those that speak Chinese :-)

[Timothy Shields]

That snippet of code is a mess. The suggestion by others that it isn't is surprising. Please see my answer for a friendlier refactor. Chained ternary operators are generally hard to read.

[phonetagger]

@TimothyShields - Chained ternary operators are actually easier to read, if formatted properly. But only if you are fluent in the language. In the chained if-else construct (e.g. your answer), the reader has to continually parse repeated assignment constructs. In the chained ternary form, they parse the assignment once and then go through the possible values that may be assigned, which is simpler.

[Timothy Shields]

@phonetagger On a fundamental level I agree with your reasoning. The point of the entire construct is to assign to good, so there should be "one equals sign." Nevertheless, I think the if-else blocks are more universally approachable. Ultimately this comes down to preference, which is probably why there are four close votes. ;)

[bbosternak]

I've made a simple tool that converts ternary expressions to if-else statements. It might help you understand more complicated ternary expressions. converter.website-dev.eu

[tinkertime]

Checking out the documentation on the ternary operator may make it even easier cplusplus.com/articles/1AUq5Di1

[Admin]

@yankee2905 you are being an elitist here. You can't expect OP to read the documentation! No way! </sarcasm>

[tinkertime]

In his defense, if you don't know the term "ternary opreator" its not very easy to google "what are these damn ? and : in my code??"

[Admin]

@yankee2905 But those symbols have English names...

[xanatos]

@yankee2905 I always thought that StackOverflow should have some search substitution mechanism able to search for non-alphabetical characters in the search and convert them to tags... Like you search for a ? and it propose [operator?]... Then we could tag these questions with [operatos?]

[Daan Timmer]

are you saying ternary operators are evil?

[rubenvb]

But what if constexpr? :-P

[Timothy Shields]

@DaanTimmer Nope. I'm saying chained ternary operators are evil. :)

[Timothy Shields]

@rubenvb Ahaha very funny. :P

[Admin]

@TimothyShields Ternary chained operators are only evil as long as they're not formatted properly.

[xanatos]

Is it true even if the operator! is overloaded? :-)

[Timothy Shields]

@xanatos I'm making a wild guess and assuming that the !x expression is really somebody's clever idea of writing x == 0 with less code. Sigh...

[mark]

Nothing wrong with this answer either... good readability (as a general practice I'd probably use the "unnecessary" braces though)

[πάντα ῥεῖ]

It's even more evil IMHO to write if/else if/else chains without putting the statements in to proper scope blocks {}!

[xanatos]

And if the operator int() always returns 0 while the operator bool() always returns true? :-) :-) :-)

[xanatos]

The important question now is... is it breakable through very evil C++ operator overloading? And is it demonstrable by a standard human being that it isn't? Or is it like the Goldbach's conjecture?

[phonetagger]

@xanatos - Noted. And edited accordingly. Actually you might note that I posted my edit for that at about the same time as you posted your comment!

[phonetagger]

@xanatos - Nice example. Of course you had to try pretty hard to get that behavior, using a pair of carefully-crafted types with meaningless conversion operators. Who would want to blindly convert any MyClass2 object to a "false" MyClass1 object without regard for the MyClass2 object's actual state? In any case, you have proved yourself (technically) correct.

[Robert Ramey]

'good = !m_seedsfilter ? true : <br/>m_seedsfilter==1 ? newClusters(Sp) : newSeed(Sp)

[vvnurmi]

Beware of "unsustainable spacing", slideshare.net/Kevlin/… . There's no need to keep adding whitespace for each nested operator, and one can argue (as Kevlin Henney does) that doing so is harmful to the readability of your code.

[xanatos]

@vvnurmi I too normally prefer a single nesting "tab" for each level, instead of vertically aligning. I prefer the "orFormat" style (going to a new line after the = is a little too much).