1603111 - Clarify whether ternary operator may be nested

Status: NEW

(Core :: DOM: File, task, P3)

Description

[Simon Giesecke [:sg] [he/him]]

In https://phabricator.services.mozilla.com/D56014#inline-343898, the question came up whether ternary operators should be nested. The downside is that this may be somewhat hard to read and may involve significant indentation and therefore a lot of line wraps. An alternative, which also allows to initialize a constant from the result, is to use an IIFE (https://www.bfilipek.com/2016/11/iife-for-complex-initialization.html), containing multiple returns.

Maybe it makes sense to state that using a single ternary operator is fine, but in more complex cases an IIFE should be preferred.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Note: I think https://phabricator.services.mozilla.com/D56014?id=203389#inline-343850 is the link you wanted for context.

(In reply to Simon Giesecke [:sg] [he/him] from comment #0)

Maybe it makes sense to state that using a single ternary operator is fine, but in more complex cases an IIFE should be preferred.

This sounds good to me as general guidance. I too have pushed back on multiple/nested ternary operators in reviews.

That said, I noticed in this particular case the issue was also being raised about using the ternary operator to invoke methods for IO side-effect purposes. I think my personal bias has always been that the ternary operator should primarily be used for functional expression purposes. Specifically, used to eliminate the introduction of temporary locals that obscure the primary intent/flow of the method. Using the ternary operator to compress imperative/procedural control flow seems obfuscating. (Perhaps because it frequently will tend to push the most meaningful invocations further to the right?)

I'd propose discussing the use of ternary operators for control flow versus initialization further.

Comment 2

[Simon Giesecke [:sg] [he/him]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #1)

Note: I think https://phabricator.services.mozilla.com/D56014?id=203389#inline-343850 is the link you wanted for context.

Yes, thanks for the better link.

(In reply to Simon Giesecke [:sg] [he/him] from comment #0)

Maybe it makes sense to state that using a single ternary operator is fine, but in more complex cases an IIFE should be preferred.

This sounds good to me as general guidance. I too have pushed back on multiple/nested ternary operators in reviews.

That said, I noticed in this particular case the issue was also being raised about using the ternary operator to invoke methods for IO side-effect purposes. I think my personal bias has always been that the ternary operator should primarily be used for functional expression purposes. Specifically, used to eliminate the introduction of temporary locals that obscure the primary intent/flow of the method. Using the ternary operator to compress imperative/procedural control flow seems obfuscating. (Perhaps because it frequently will tend to push the most meaningful invocations further to the right?)

I'd propose discussing the use of ternary operators for control flow versus initialization further.

I'm not sure if I completely get your point here. What does "for control flow" exactly mean?

Probably we should have examples when adding that to the style document to avoid ambiguity.

As said on the review, I generally tend to prefer functional-style ?: over imperative-style if/else constructs (I really like that in languages like Rust and Xtend everything is an expression, though I still prefer the ternary operator syntax over expression if/else somehow). One thing that might bias this a bit is that in C++ with exceptions, I wouldn't use the ternary operator for functions returning void, which are called solely for their side effect. Here, and I think this was also the case in the review, the functions just return a nsresult error code, which is conceptually equivalent to the former case, but not syntactically. If that matches what you meant, I'd say agree that the ternary operator might be discouraged for cases where the functions are called only for their side effect, i.e. which return void or nsresult.

Restricting the use of the ternary operator to pure functions would be too restrictive in my opinion, and also hard to decide on, since C++ makes no syntactical differences between functions with and without side effects.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

My feelings on this are nebulous. I mainly want to express that in general I find the following more quickly comprehensible:

if (inOuterSpace) {
  doOuterSpaceThing();
} else {
  doLandThing();
}

than

inOuterSpace ? doOuterSpaceThing() : doLandThing();

or

(inOuterSpace && doOuterSpaceThing()) || doLandThing();

These examples aren't actually so bad; it's more when the source starts getting more dense because of arguments, etc.

This isn't something I care about deeply and in general I'd defer to the patch author except where the conditions at play become hard to understand. I think we should be moving to rust as much as possible and in rust we'd probably just use match blocks that have an understandable density level.