Defending the ternary operator

[Roy Smith]

Andrew Koenig <ark at research.att.com> wrote:

> EXAMPLE 1:
> 
>         p = <null-terminated string>;
>         q = <fixed-length char array>;
>         for (i = 0; i < sizeof(<fixed-length char array>); i++) {
>             if (*q++ != (*p? *p++: ' ')) {
>                    /* Complain that the two strings aren't equal */
>             }
>         }
> 
> As a C programmer, I have no trouble understanding what's going on:
> The "if" compares the next character in q with either the next
> character in p or a blank, depending on whether we've hit the null
> that terminates p.

I find the above code horribly confusing.  I've got 25 years of C coding 
experience.  I know I certainly wouldn't want to use anything I wrote 20 
years ago as an example of how things should be done today :-)

One thing that's changed about how I write code today vs. 20 years ago 
is that back then, I was a pure C hacker.  I was head-down in C code 8 
hours a day, 5 days a week.  I could rattle off every nuance of every 
minor language feature, and prided myself on writing the most compact 
and efficient code that was possible.  Today I use C, C++, Python, Perl, 
TCL, shell, even a little Lisp, and I'm starting to look at Java.  
Choice of language is often driven by external constraints.  I no longer 
have the luxury of knowing every nuance and idiom of every language I 
use.  This makes me tend to shy away from languages where you cannot be 
effective without being a language lawyer.

OK, back to the code fragment in question.

I'm reading the condition of the if statement trying to understand it.  
First I see *q++.  OK, that's a common idiom and I instantly understand 
that you're walking a pointer through an array. Then I see !=.  OK, 
you're comparing it to something.  Then I see the conditional and have 
to push my mental stack on level and say, "OK, let me understand what's 
going on here, and then I'll come back and figure out how it fits with 
what I've already understood".  That's where it gets confusing.  My 
first pass at rewriting the for loop would be something like:

      for (...) {
         temp = *p ? *p++ : ' ';
         if (*q++ != temp) {
            /* complain */
         }
      }

which at least lets me understand the two things sequentially.  First, 
I'm getting a character from p (with blank-padding).  Second, I'm 
comparing that to a character from q.  Next, I think I'd try to invent 
some more descriptive variable names for p, q, and temp.

But, all I've done so far is nit-picked your style, and my version still 
uses the conditional, which I'm arguing against.  So, I think my next 
step would be to rewrite it again, like this:

      for (...) {
         if (*q++ != getNextCharWithBlankPadding (&p)) {
            /* complain */
         }
      }

      char getNextCharWithBlankPadding (char **p)
      {
         if (**p)
            return *p++;
         else
            return ' ';
      }

Of course this is less efficient than it was before (assuming the 
optimizer doesn't in-line the function call), but until somebody 
convinces me with real profiling data, I'm much more interested in 
readability than efficiency.  The pointer-to-a-pointer doesn't rank very 
high on my readability scale, but that's a problem forced on us by C, 
which wouldn't show up in Python.

Fundamentally, you're doing a complex set of things.  You're walking two 
different arrays in parallel, comparing them element by element, and 
performing blank-padding on one of them.  Squashing all that into one 
line certainly makes it more compact, but it doesn't make it easier to 
understand.

> EXAMPLE 2:
> 
>         mode = cvlong(param, strlen(param), 8) |
>             (c == 'c'? S_IFCHR: S_IFBLK);
> 
> Before you flame at me for using an absolute constant here, I should
> point out that the "8" in the argument to cvlong means "Treat this
> as an octal number, please."  Again, without ?: I might have written
> 
>         mode = cvlong(param, strlen(param), 8);
>         if (c == 'c')
>             mode |= S_IFCHR;
>         else
>             mode |= S_IFBLK;

You seem to thing this is a problem, but in my opinion, your code would 
have been easier to understand written the second way.

> EXAMPLE 5:
>        fprintf(file,
>            *p >= '0' && *p <= '7'? "\\%.3o": "\\%o",
>            c);
> This code is a little sneaky.

I don't like sneaky code.

> Again, let's brush aside questions about what the code is trying to do,
> and consider how it does it.  Without ?:, I think it would look like this:
> 
>         if (*p >= '0' && *p <= '7')
>             fprintf(file, "\\%.3o", c);
>         else
>             fprintf(file, "\\%o", c);

Personally, I would have refactored this as something like:

   format = "\\%o";
   if (*p >= '0' && *p <= '7')
      format = "\\%.3o";
   fprintf (file, format, c);

and I think the result is better (easier to understand) than either of 
your alternatives.