clean-code-javascript icon indicating copy to clipboard operation
clean-code-javascript copied to clipboard

Аvoid mental mapping

Open NikitaIT opened this issue 4 years ago • 4 comments

https://github.com/ryanmcdermott/clean-code-javascript#avoid-mental-mapping

Remove or edit rule as:

Bad:

doSomeStuffWithLocation(location);

function doSomeStuffWithLocation(l) {
  doStuff();
  doSomeOtherStuff();
  // ...
  // ...
  // ...
  dispatch(l);
}

Good:

doStuff();
doSomeOtherStuff();
doSomeStuffWithLocation(location);
dispatch(location);

function doSomeStuffWithLocation(location) {
  // ...
  // ...
  // ...
}

I can't think of straight away how to reflect this correctly, but the way it is brought up is confusing.

This lambda rule is misleading.

const locations = ["Austin", "New York", "San Francisco"];
  // This code is not clean.
locations.forEach(location => { 
  // This code is not clean, too.
  doStuff();
  doSomeOtherStuff();
  // ...
  // ...
  // ...
  dispatch(location);
});

In fact, in this case, we have to move the function out by making it named. Or apply a different refactoring.

const locations = ["Austin", "New York", "San Francisco"];
locations.forEach(doSomeStuff);

A correct example would be something like:

Bad:

const locations = [{ id: 1, name: "Austin" }, { id: 2, name: "New York" }, { id: 3, name: "San Francisco" }];
const locationIds = locations.map(location => location.id);

Good:

const locations = [{ id: 1, name: "Austin" }, { id: 2, name: "New York" }, { id: 3, name: "San Francisco" }];
const locationIds = locations.map(x => x.id);

In this case, we consider functions as a workflow, and it does not matter to us exactly how the parameter is named.

locations // map locations
  .map(x => x.id) // to id

Or a more complex example:

Bad:

users // map users
  .flatMap(user => user.orders) // to flat user orders
  .flatMap(order => order.payments) // to flat order payments

And the developers read it as:

users
 .flat ... user => user.orders
 .flat ... order => order.payments

Good:

users // map users
  .flatMap(x => x.orders) // to flat orders
  .flatMap(x => x.payments) // to flat payments

And the developers read it as:

users
 .flat ... .orders
 .flat ... .payments 

Or just:
users.orders.payments 

Using an abstract x reduces the read overhead since the beginning of the expression x => x. ignored.

Many developers may find this strange, but we never actually read the names of variables in lambda functions, and we almost never write lambda functions longer than a couple of lines.

They also usually ask what to do with nested functions. It all depends on the situation here, but my adherence is that if nesting appears, it is wise to move it out of the chain. As in https://github.com/ryanmcdermott/clean-code-javascript#functions-should-do-one-thing

NikitaIT avatar Jan 19 '21 22:01 NikitaIT

Good

Serhiomood avatar Sep 10 '21 14:09 Serhiomood

It's strange to see so many dislikes. I constantly see people using meaningful names in lambdas don't change them when refactoring because they don't really mean anything.

NikitaIT avatar Nov 04 '21 18:11 NikitaIT

locations.forEach(l => {
  doStuff();
  doSomeOtherStuff();
  // ...
  // ...
  // ...
  dispatch(l);
});

Anyone knows any ESLINT rule to prevent single character in arguments?

creativemind1 avatar Jan 20 '22 19:01 creativemind1

locations.forEach(l => {
  doStuff();
  doSomeOtherStuff();
  // ...
  // ...
  // ...
  dispatch(l);
});

Anyone knows any ESLINT rule to prevent single character in arguments?

  1. http://astexplorer.net/
  2. https://eslint.org/docs/rules/no-restricted-syntax

I think you could do something similar to this:

{
    "rules": {
        "no-restricted-syntax": [
            "error",
            {
                "selector": "ArrowFunctionExpression[params[0] && params[0].name && params[0].name.length == 1]",
                "message": "Single character arguments names are not allowed for arg[0]."
            },
            {
                "selector": "ArrowFunctionExpression[params[1] && params[1].name && params[1].name.length == 1]",
                "message": "Single character arguments names are not allowed for arg[1]."
            }
        ]
    }
}

if u use Typescript ast:


            { 
                "selector": "ArrowFunctionExpression[parametrs[0].getText().length == 1]",
                "message": "Single character arguments names are not allowed for arg[0]."
            },
            { 
                "selector": "ArrowFunctionExpression[parametrs[1].getText().length == 1]",
                "message": "Single character arguments names are not allowed for arg[1]."
            },

TS ast:

                 // for ArrowFunctionExpression: (a, { a, b, c }) => {...}
                 // if { name: Identifier } then .getText() === "a"
                 // if { name: ObjectBindingPattern } then .getText() === "{ a, b, c }"

Babel/eslint ast:

                 // for ArrowFunctionExpression: (a, { a, b, c }) => {...}
                 // if { name: "a" }
                 // if { name: undefined  }

I must warn you that I disclaim responsibility for suffering from this rule.

NikitaIT avatar Jan 29 '22 23:01 NikitaIT