How to rewrite complicated lines of C++ code (nested ternary operator)
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.
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.
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).
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).