Is this a reasonable use of the ternary operator?

Question

Are there any understanding / maintainability issues that result from code like

inVar1 == 0 ? NULL : v.push_back(inVar1);
inVar2 == 0 ? NULL : v.push_back(inVar2);

and so forth.

The possibly confusing idea is using the ternary operator for program flow rather than variable assignment, which is the usual explanation.

I haven't seen coding standards at work that address this usage, so while I'm comfortable doing this I'd like to find out if there is a good reason not to.

Answer 1

I think this should be avoided. You could use a 1-line if statement in its place.

if(inVar1 != 0) v.push_back(inVar1);

Answer 2

I think it's confusing and a lot harder to read than simply typing;

if (inVar != 0)
  v.push_back(inVar);

I had to scan your example several times to figure out what the result would be with any certainty. I'd even prefer a single-line if() {} statement than your example - and I hate single-line if statements :)

Answer 3

The ternary operator is meant to return a value.

IMO, it should not mutate state, and the return value should be used.

In the other case, use if statements. If statements are meant to execute code blocs.

Answer 4

I think you would be better served in doing a proper if structure. I even prefer to always have braces with my if structures, in the event I have to add lines later to the conditional execution.

if (inVar != 0) {
    v.push_back(inVar);
}

Answer 5

Compilers these days will make an if as fast as a ternary operator.

You goal should be how easy is it for another software developer to read.

I vote for

if ( inVar != 0 )
{
   v.push_back( inVar );
}

why the brackets...because one day you may want to put something else in there and the brackets are pre-done for you. Most editors these days will put them in anyway.

Answer 6

While, in practice, I agree with the sentiments of those who discourage this type of writing (when reading, you have to do extra work to scan the expression for its side effects), I'd like to offer

!inVar1 ?: v.push_back(inVar1);
!inVar2 ?: v.push_back(inVar2);

...if you're going for obscure, that is. GCC allows x ?: y in place of x ? x : y. :-)

Answer 7

Your use of the ternary operator gains you nothing and you hurt the codes readability.

Since the ternary operator returns a value that you are not using it is odd code. The use of an if is much more clear in a case like yours.

Answer 8

The ternary is a good thing, and I generally promote it's usage.

What you're doing here however tarnishes it's credibility. It's shorter, yes, but it's needlessly complicated.

Answer 9

As litb mentioned in the comments, this isn't valid C++. GCC, for example, will emit an error on this code:

error: `(&v)->std::vector<_Tp, _Alloc>::push_back [with _Tp = int, _Alloc =
std::allocator<int>](((const int&)((const int*)(&inVar1))))' has type `void' 
and is not a throw-expression

However, that can be worked around by casting:

inVar1 == 0 ? (void)0 : v.push_back(inVar1);
inVar2 == 0 ? (void)0 : v.push_back(inVar2);

But at what cost? And for what purpose?

It's not like using the ternary operator here is any more concise than an if-statement in this situation:

inVar1 == 0 ? NULL : v.push_back(inVar1);
if(inVar1 != 0) v.push_back(inVar1);

Answer 10

I use ternary operator when I need to call some function with conditional arguments - in this case it is better then if.

Compare:

printf("%s while executing SQL: %s",
        is_sql_err() ? "Error" : "Warning", sql_msg());

with

if (is_sql_err())
    printf("Error while executing SQL: %s", sql_msg());
else
    printf("Warning while executing SQL: %s", sql_msg());

I find the former is more appealing. And it complies to DRY principle, unlike latter - you don't need to write two nearly identical lines.

Answer 11

If you have multiple method invocations in one or both of the tenary arguments then its wrong. All lines of code regardless of what statement should be short and simple, ideally not compounded.

Answer 12

A proper if statement is more readable, as others have mentioned. Also, when you're stepping through your code with a debugger, you won't be able to readily see which branch of an if is taken when everything is in one line or you're using a ternary expression:

if (cond) doIt();

cond ? noop() : doIt();

Whereas the following is much nicer to step through (whether you have the braces or not):

if (cond) {
    doIt();
}

Answer 13

I think that sometimes the ternary are a necessary evil in initializer lists for constructors. I use them mostly for constructors where I want to allocate memory and set some pointer to point at it before the body of the constructor.

An example, suppose you had an integer storage class that you wanted to have take a vector as an input but the internal representation is an array:

class foo
{
public:
    foo(std::vector<int> input);
private:
    int* array;
    unsigned int size;
};

foo:foo(std::vector<int> input):size(input.size()), array( (input.size()==0)?
        NULL : new int[input.size])
{
    //code to copy elements and do other start up goes here
}

This is how I use the ternary operator. I don't think it is as confusing as some people do but I do think that one should limit how much they use it.

Answer 14

Most of the tortured ternaries (how's that for alliteration?) I see are merely attempts at putting logic that really belongs in an if statement in a place where an if statement doesn't belong or can't go.

For instance:

if (inVar1 != 0)
  v.push_back(inVar1);
if (inVar2 != 0)
  v.push_back(inVar2);

works assuming that v.push_back is void, but what if it's returning a value that needs to get passed to another function? In that case, it would have to look something like this:

SomeType st;
if (inVar1 != 0)
  st = v.push_back(inVar1);
else if (inVar2 != 0)
  st = v.push_back(inVar2);
SomeFunc(st);

But that's more to digest for such a simple piece of code. My solution: define another function.

SomeType GetST(V v, int inVar1, int inVar2){
    if (inVar1 != 0)
      return v.push_back(inVar1);
    if (inVar2 != 0)
      return v.push_back(inVar2);        
}

//elsewhere
SomeFunc(GetST(V v, inVar1, inVar2));

At any rate, the point is this: if you have some logic that's too tortured for a ternary but will clutter up your code if it's put in an if statement, put it somewhere else!

Answer 15

As mentioned, it's not shorter or clearer than a 1 line if statement. However, it's also no longer - and isn't really that hard to grok. If you know the ternary operator, it's pretty obvious what's happening.

After all, I don't think anyone would have a problem if it was being assigned to a variable (even if it was mutating state as well):

var2 = inVar1 == 0 ? NULL : v.push_back(inVar1);

The fact that the ternary operator always returns a value - IMO - is irrelevant. There's certainly no requirement that you use all return values...after all, an assignment returns a value.

That being said, I'd replace it with an if statement if I ran across it with a NULL branch.

But, if it replaced a 3 line if statement:

if (inVar == 0) {
   v.doThingOne(1);
} else {
   v.doThingTwo(2);
}

with:

invar1 == 0 ? v.doThingOne(1) : v.doThingTwo(2);

I might leave it...depending on my mood. ;)

Answer 16

inVar1 != 0 || v.push_back(inVar1);
inVar2 != 0 || v.push_back(inVar2);

common pattern found in languages like Perl.