Coding style

[narcis-radu]

Hi all,
I wanted to get the community preference on coding style and make sure we use the patterns consistently in Milo. Here are some patterns I wanted to discuss, right now all options are used in random scripts.

1. fail fast

if (something) {
 const x = y;
 doSomething();
}

OR

if (!something) return;
const x = y;
doSomething();

2. inline if

if (something) {
  doSomething();
}

OR

if (something) doSomething();

[mokimo]

IMO option 2 for both:

// Encourages "one-level", no nested if-else-if-if-if-if-else
if (!something) return;
const x = y;
doSomething();

AND

if (something) doSomething();

I wonder if we can adapt the linter or prettier rules to enforce on what we decide for?

[npeltier]

i think that this would be ideal (to have linter/prettier rules for style consensus)

[narcis-radu]

My preference:

if (!something) return;
const x = y;
doSomething();

and

if (something) doSomething();

[3ch023]

Hi, depending on the situation I use both

if (something) {
 const x = y;
 doSomething();
}

and

if (!something) return;
const x = y;
doSomething();

in certain cases negating 'something' makes it harder to understand, then I use option 1. In most cases, I use option 2.

Imo, having both styles doesn't make code harder to understand. What is the purpose of standardizing?

Can it be enforced automatically? (via prettier/linter as @mokimo mentioned above) In this case, it wouldn't require more effort from developers.

[chrischrischris]

I think that if we choose a style we should try to stick to it regardless of negation.

[yesil]

I don't like the following syntax as it makes it harder to debug w.r.t breakpoints.

if (condition) doSingleStatement();

The more verbose version gives more air to the code between logical blocks :)

if (condition) {
    doSingleStatement();
}

I would support keeping both versions.

However, I agree that we should standardize our coding style. We should also enhance our linter to catch these minor violations before committing, to avoid wasting time on PR reviews for such issues.

[narcis-radu]

Most of the Milo code uses the inline notation already and so far things has been easy to debug, Chrome is really helpful when it comes to setting breakpoints, see below.

I think all of us agree that the linter should dictate the format, we'll spend some time to check that.

[overmyheadandbody]

1

if (!something) return;
const x = y;
doSomething();

One less line (although a couple more characters due to the return) and one less indenting level, which makes the code much easier to read. I saw a lot of places where we have 6-7 tabs before a line of code due to indenting and it makes it difficult to follow at that point.

2

if (something) doSomething();

Same arguments, two lines less, no additional indenting, code is easier to read. As @narcis-radu mentioned, Chrome doesn't have an issue with this and you can easily access the function itself.

[mokimo]

As for the linting rule, this is what we search for https://eslint.org/docs/latest/rules/curly - given what we've discussed so far.

Particularly this example ... this does introduce a lot of issues, hence I'd suggest we put it as 'warn' rather than 'error'

[yesil]

Thanks for this post. I value the consensus and hopefully we can make it easier with the linter.
We will align our coding style with Milo as we update.

[chrischrischris]

Looks like there's consensus on the styles (which I agree with). The other big issue is that we have too much code that is currently breaking the linting. Until that is resolved it makes it much harder to enforce rules. Also once resolved we should have the linter run during an npm run test as most devs don't remember to run the linter by itself.

[narcis-radu]

Thank you all! I have created a follow-up JIRA story to address this - https://jira.corp.adobe.com/browse/MWPW-158751