Wp-calypso: Question: Conditionals & Boolean Expression Code Style

[spen]

Current

How are we feeling about logic like the below?

const isNewSite = action.isJetpack && ( action.siteId !== state.get( 'currentSiteId' ) );
_(client/state/themes/themes/reducer.js)_

Personally I find it be a bit of a speed-bump when reading logic (albeit a very slight one in this case).
This example is a bit trivial but could still be improved...

Suggested

In previous projects I've pushed for a set up like the one below.

const isCurrentSite = action.siteId === state.get( 'currentSiteId' );
const isNewSite = action.isJetpack && !isCurrentSite;

I aim to have conditionals & boolean logic read as close to English as possible (Is new site if it is Jetpack and it is not current site).

Reasoning

I find it helps with readability but more importantly shows the intent more clearly.
If the developer creates the variable 'isCurrentSite' you can be sure that they want to check that the site is the current one.
It might even turn out that this logic is actually wrong - but, now that we know the intent, the fix should be easier to make.
I think it also makes the author think more about what it is they are trying to define... Is it really semantic to define the site as 'new' because it (isJetpack && !isCurrentSite)?

I've just written a little blog post touching on this subject with an example that might be helpful:
https://spentaylor.wordpress.com/2016/10/01/31/

Actions:

If pulling out long conditional logic in to smaller, more digestible chunks is preferred I suggest this is mentioned in the JS code standards doc
I'm happy to do the PR, if this is something that others agree on.

Thanks!

[lamosty]

I'm all for better and easier readability. I think your example demonstrates the issue well.

Let's hear from some other folks as well before mentioning this in JS code standards/making it a-must, though. :)

/cc @gwwar @aduth @mtias @ehg

[aduth]

I personally feel this is one of those gray areas that is better left to a developer's best judgment, not to be imposed one way or the other in our standards. Even in your own example, it's unclear just how much to break it down. In other words, is this even more clear?

const siteId = action.siteId;
const currentSiteId = state.get( 'currentSiteId' );
const isJetpack = action.isJetpack;
const isCurrentSite = siteId === currentSiteId;
const isNewSite = isJetpack && ! isCurrentSite;

How are we to describe at what point assignment might cause it to become less readable? I think your suggested code strikes a good balance, but in general this seems difficult to standardize.

[spen]

Makes sense! Putting an actual defined rule would be tricky - I hadn't actually got that far with my thinking!
Thanks for taking a look @lamosty & @aduth :D