1

Is there a simple heuristic for understanding how to read nested ternary operators? I came across this in someone's source code and can't grok it. A simple ternary is easy:

isRed = color == 'red' ? true : false

But how do you read the following? Can I just line up the first with the last and the second with the second to last, or must I parse this into an if/else tree in my head?

var offset =
  ( hasFrozenRows )
    ? ( options.frozenBottom )
    ? ( row >= actualFrozenRow )
    ? ( h < viewportTopH )
    ? ( actualFrozenRow * options.rowHeight )
    : h
    : 0
    : ( row >= actualFrozenRow )
    ? frozenRowsHeight
    : 0
    : 0;

Retabbed, it can look like this, which is almost understandable (?)

      var offset =
        ( hasFrozenRows ) ?
          ( options frozenBottom ) ?
            ( row >= actualFrozenRow ) ?
              ( h < viewportTopH ) ?
                ( actualFrozenRow * options.rowHeight )
                :
                h
              :
              0
            :
            ( row >= actualFrozenRow ) ?
              frozenRowsHeight
              :
              0
            :
            0;
SimplGy
  • 20,079
  • 15
  • 107
  • 144
  • 8
    If I ran the zoo, that would _never_ make it past a code review. – Alex Wayne Apr 10 '14 at 19:28
  • The minimum would be to indent, but it will still be unreadable. – Raphaël Althaus Apr 10 '14 at 19:29
  • 1
    No need for the parentheses: `isVisible = isOnScreen && notHidden ? true : false`. But why are you doing that anyway? It's just `isVisible = isOnscreen && notHidden`. But your underlying problem here is that the basic logic is so contorted that it will be throwing up bugs from here to eternity. Best to refactor this at a higher level. –  Apr 10 '14 at 19:31
  • no argument there, @torazaburo. I'm just trying to understand the intent of somebody else's code. – SimplGy Apr 10 '14 at 19:35
  • This is just another way of obsfucating the code. – GameAlchemist Apr 10 '14 at 20:11

3 Answers3

4

I think you could have more luck if you try to read it as a series of check this, if true then this, else that.

For this, it might be easier to put the ? and : operators at the beginning of lines, and read them as if they were arrows on a flowchart, labeled "yes" and "no". e.g.:

cond1 
  ? cond2 
    ? cond3 
      ? res1 
      : res2
    : res3
  : res4

Which could be read as:

cond1?
  yes -> is cond2 
    yes -> is cond3?
      yes -> res1 
      no -> res2
    no -> res3
  no -> res4

That still doesn't make this very readable, and I agree with all the comments saying this kind of code should really be rewritten to be readable.

Avish
  • 4,516
  • 19
  • 28
2

For something messy like this you have to work from the outside in. Essentially, what this maps to is this:

if ( hasFrozenRows ) {
  if ( options.frozenBottom ) {
    if ( row >= actualFrozenRow ) {
      if ( h < viewportTopH ) {
        return ( actualFrozenRow * options.rowHeight )
      } else {
        return h;
      }
    } else {
      return 0;
    }
  } else {
    if ( row >= actualFrozenRow ) {
      return frozenRowsHeight
    } else {
      return 0
    }
  }
} else {
  return 0;
}

And I thought the legacy code I worked with was a nightmare...

I recommend running this through a unit tester, like Jasmine, and compare the outputs with the original code to confirm that it's the same.

Pete
  • 3,246
  • 4
  • 24
  • 43
  • Do you really think that nested if-else statements are less messy? – Bergi Apr 10 '14 at 20:16
  • Less "messy"? Maybe not. But at least this version makes the logic understandable to a feeble human. – Alex Wayne Apr 10 '14 at 20:41
  • @Bergi it's not pretty, but I didn't have the time to go through the logic for him to simplify it. Off the bat, I can see some redundancy and would probably fix it if it was my own problem. – Pete Apr 10 '14 at 20:56
  • 1
    Also, once it's in this format you can more easily see how to refactor it, which is how this ugliness will eventually get solved. – Alex Wayne Apr 10 '14 at 20:58
  • I don't see that point. The semantics are exactly the same, a reasonably indented ternary-operator-tree is more readable because of less syntax overhead. It's even easier to refactor because of fewer characters to type. – Bergi Apr 10 '14 at 23:07
2

must I parse this into an if/else tree in my head?

Yes, because in this case it is a tree and not simply chained operators. Those would be easy to understand :-) Without indentation, this definitely needs refactoring.

In this particular case, it also would be helpful to negate the conditions, making reading a lot easier as it puts the condition and the effect directly next to each other:

var offset = (!hasFrozenRows)
               ? 0
               : (!options.frozenBottom)
                 ? (row < actualFrozenRow)
                   ? 0
                   : (h < viewportTopH)
                     ? actualFrozenRow * options.rowHeight
                     : h
                 : (row >= actualFrozenRow)
                   ? frozenRowsHeight
                   : 0;

We also could move the duplicate row >= actualFrozenRow comparison one level up:

var offset = (!hasFrozenRows)
             ? 0
             : (row < actualFrozenRow)
               ? 0
               : (!options.frozenBottom)
                 ? frozenRowsHeight
                 : (h < viewportTopH)
                   ? actualFrozenRow * options.rowHeight
                   : h;

…which actually renders it completely understandable, even if not negated:

var offset = ( hasFrozenRows )
             ? ( row >= actualFrozenRow )
               ? ( options.frozenBottom )
                 ? ( h < viewportTopH )
                   ? actualFrozenRow * options.rowHeight
                   : h
                 : frozenRowsHeight
               : 0
             : 0;

You now can also see that you might merge the first two conditions to hasFrozenRows && row >= actualFrozenRow.

Bergi
  • 630,263
  • 148
  • 957
  • 1,375