Skip to content

Update README.md #2430

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -452,9 +452,14 @@ Other Style Guides
if (subject === 'Mockingbird') {
return author === 'Harper Lee';
}

return false;
});

//best
inbox.filter(({ subject, author } ) => {
return subject === 'Mockingbird' && author === 'Harper Lee';
});

Comment on lines +457 to +462
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't actually best - our linter config will warn on it. it'd be:

Suggested change
//best
inbox.filter(({ subject, author } ) => {
return subject === 'Mockingbird' && author === 'Harper Lee';
});
//best
inbox.filter(({ subject, author }) => subject === 'Mockingbird' && author === 'Harper Lee');

however, then we've defeated the whole point of this section, which is showing patterns around explicit return in callbacks.

so, i think this is best reverted.

```

<a name="arrays--bracket-newline"></a>
Expand Down Expand Up @@ -522,6 +527,10 @@ Other Style Guides
function getFullName({ firstName, lastName }) {
return `${firstName} ${lastName}`;
}

// best - 2
const getFullName = ({ firstName, lastName }) => `${firstName} ${lastName}`;

```

<a name="destructuring--array"></a><a name="5.2"></a>
Expand Down Expand Up @@ -766,6 +775,19 @@ Other Style Guides
function handleThings(opts = {}) {
// ...
}

// best
function isNullOrUndefined(obj){
// '==' handles both undefined and null
return obj == null;
}
Comment on lines +779 to +783
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shouldn't ever be a function, it should just be inline.

function getDefaultValueIfNotPresent(obj,defaultValue){
return isNullOrUndefined(obj) ? defaultValue : obj;
}
Comment on lines +784 to +786
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function getDefaultValueIfNotPresent(obj,defaultValue){
return isNullOrUndefined(obj) ? defaultValue : obj;
}
function getDefaultValueIfNotPresent(obj,defaultValue) {
return isNullOrUndefined(obj) ? defaultValue : obj;
}

this is entirely unnecessary, though, since ?? exists - you can just do obj ?? defaultValue, without any need for a function.


function handleThings(opts = getDefaultValueIfNotPresent(opts,{})){
// ...
}
Comment on lines +788 to +790
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think this adds anything over the existing example of function handleThings(opts = {}) {.

Suggested change
function handleThings(opts = getDefaultValueIfNotPresent(opts,{})){
// ...
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you explicitly pass null to handleThings function then opts will be null rather than the default {} value. Therefore I think we should also handle this case.

```

<a name="functions--default-side-effects"></a><a name="7.8"></a>
Expand Down Expand Up @@ -951,6 +973,9 @@ Other Style Guides
const y = x + 1;
return x * y;
});

// best
[1, 2, 3].map((x) => x * (x + 1));
```
Comment on lines +976 to 979
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fine, but it may not actually be better - the "y" might add clarity, depending on what the code is doing.


<a name="arrows--implicit-return"></a><a name="8.2"></a>
Expand Down