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

Update README.md #2430

wants to merge 1 commit into from

Conversation

chety
Copy link

@chety chety commented May 24, 2021

  • Enhance method implementation by returning in a single line rather than using if and else.
  • Add support for the default parameter value.

Comment on lines +457 to +462

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

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.

Comment on lines +779 to +783
// best
function isNullOrUndefined(obj){
// '==' handles both undefined and null
return obj == null;
}
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.

Comment on lines +784 to +786
function getDefaultValueIfNotPresent(obj,defaultValue){
return isNullOrUndefined(obj) ? defaultValue : obj;
}
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.

Comment on lines +976 to 979

// best
[1, 2, 3].map((x) => x * (x + 1));
```
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.

Comment on lines +788 to +790
function handleThings(opts = getDefaultValueIfNotPresent(opts,{})){
// ...
}
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.

@ljharb ljharb marked this pull request as draft May 24, 2021 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants