Recommended ?: refactoring is worse

[erights]

https://github.com/airbnb/javascript#comparison--nested-ternaries

The recommended refactoring of

const foo = maybe1 > maybe2
  ? "bar"
  : value1 > value2 ? "baz" : null;

into

const maybeNull = value1 > value2 ? 'baz' : null;
const foo = maybe1 > maybe2 ? 'bar' : maybeNull;

is not equivalent. In the latter, the value1 > value2 comparison is always evaluated, and one of 'baz' or null is also always evaluated. In this case, it is probably fine, but in general it has two problems:

• These extra evaluations may be expensive
• These extra evaluations may have side effects or throw when maybe1 > maybe2.

[ljharb]

A > evaluation will never be expensive; regardless, performance is always less of a concern than clarity.

Similarly, if you're following the spirit of this guide, such a comparison would never have side effects nor throw, so in practice this is a non-issue.

[erights]

If your recommendation was specific to operations like > which are cheap, assumed side effect free, and assumed not to throw, no argument. However, I took your text to apply as well to

const foo = cond1()
  ? val1()
  : cond2() ? val2() : val3();

[ljharb]

It does. However, if an operation is expensive, it should be stored in a separate variable to call it out, rather than being inlined in the middle of an innocuous-seeming operation.

[erights]

I disagree with this rationale. Even < is not actually safe, even though it naively looks safe at the call site. It may have been safe when the call was written, but becomes unsafe under maintenance when code elsewhere is changed.

const operand = {toString(){throw 'x'}, valueOf(){throw 'y'}};
...
... operand < 3 ...

Saying that "the code elsewhere should not have been changed this way" is no excuse for ignoring the possibility at the call site.

Your recommendation text should at least warn about the potential non-equivalences of the recommended refactoring.

[ljharb]

This is a style guide, not a library or a specification; it doesn’t have to be concerned about things that simply don’t happen in practice (including defining custom valueOf/toString methods).

If an operation throws, that’s fine. If it’s expensive, it should be stored in a variable and named so as to indicate that it’s necessary to cache it.

The spirit of this guide requires explicitly coercing most everything prior to using it; if using > on non-number types, they should be converted to numbers first, making such comparisons always fast and safe.