current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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;
share|improve this question

3 Answers 3

up vote 1 down vote accepted

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.

share|improve this answer
up vote 1 down vote

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.

share|improve this answer
up vote 2 down vote

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 ...

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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