3
\$\begingroup\$

This code gets a state from a query string hash part, converts the hash part to a JavaScript object and then tries to get a view, dashboard or a mode value. If those keys are not present in object, it returns $.pulse.qs.views.home.

Does this JavaScript code with ternary operator follow common best practices? It looks like ternary operator abuse and it is not readable. Or is it just me?

var state = $.query.getState();
var screen = state[$.pulse.qs.view]
    ? state[$.pulse.qs.view]
    : state[$.pulse.qs.dashboard] || state[$.pulse.qs.mode]
        ? $.pulse.qs.views.dashboard
        : $.pulse.qs.views.home;
\$\endgroup\$
1
\$\begingroup\$

I'm not aware of a definitive standard concerning the reasonable limits of the ternary operator, in any language. The general rule of thumb is to avoid the ternary operator when it hurts readability, but that's subjective and may change from person to person, team to team. See also this related discussion.

What can be said fairly objectively, the ternary operator is best when:

  • Used without nesting (single ternary)
  • Used for a trivial simplification in a perfectly clear way

As the first answer on the linked question points out, if you find the code unreadable, then it's probably time to change it. But this is a matter of taste. I'm on the fence, for example: thanks to the indentation, the code seems still readable to me.

If it's not up to you to change and rewrite with nicely spelled out if-else statements, then that's tough luck, because I can't really think of a compelling argument to prove that this piece of code is fundamentally wrong.

\$\endgroup\$
2
\$\begingroup\$

I think a good practice is to name what you mean, cause that makes the code more readable. So I would create a var for each state and grap a good name: stateOfViewAvailable ...

\$\endgroup\$
1
\$\begingroup\$

If you want to use ternary operators to define screen, I suggest reformulating it this way, as a linear chain without a nested || operator:

var screen = state[$.pulse.qs.view]      ? state[$.pulse.qs.view]
           : state[$.pulse.qs.dashboard] ? $.pulse.qs.views.dashboard
           : state[$.pulse.qs.mode]      ? $.pulse.qs.views.dashboard
                                         : $.pulse.qs.views.home;

You can make sense of it more easily by scanning left-to-right, then top-to-bottom. In my opinion, it's more compact and readable than a series of if-elses.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.