Skip to content
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

isArray has unnecessary code #192

Closed
kirilloid opened this issue Dec 16, 2017 · 8 comments
Closed

isArray has unnecessary code #192

kirilloid opened this issue Dec 16, 2017 · 8 comments

Comments

@kirilloid
Copy link

kirilloid commented Dec 16, 2017

Why do we need that extra !!?

[false, null, undefined, NaN, 0, ''].filter(Array.isArray)

is empty, no falsy value is considered as an array

Maybe, I can create a PR with several such small changes instead of writing about every separately?

@kirilloid kirilloid changed the title isArray isArray has unnecessary code Dec 16, 2017
@Chalarangelo
Copy link
Owner

To convert to a boolean value. This was mentioned before and was actually PRed to fix the issue of the return value not always being boolean.

@kirilloid
Copy link
Author

kirilloid commented Dec 16, 2017

But you don't need !!val && (or val &&) part at all! Just use Array.isArray

@Chalarangelo
Copy link
Owner

You need it to check if the array is empty or something like that.

@kirilloid
Copy link
Author

No, it is not needed.
There are only six falsy (which result in false when converted to Boolean, e.g. with !!) values in JavaScript, all enumerated in my first comment.

@dfdeagle47
Copy link

To add more context, the PR in question is #130 which fixes #111, i.e.

const isArray = val => val && Array.isArray(val);
// isArray(null) -> null (not false)

which was changed to

const isArray = val => !!val && Array.isArray(val);
// isArray(null) -> false

However, as @kirilloid just said, the first part of the expression is unneeded, it should read

const isArray = val => Array.isArray(val);

Unless I'm unaware of some implementation difference between different engines, Array.isArray() always returns a boolean value, and correctly works with the different falsy values.

dfdeagle47 added a commit to dfdeagle47/30-seconds-of-code that referenced this issue Dec 22, 2017
@dfdeagle47 dfdeagle47 mentioned this issue Dec 22, 2017
13 tasks
@Chalarangelo
Copy link
Owner

@dfdeagle47 there's actually some controversy regarding this particular snippet, as there is a slight chance we might remove it entirely iff we only need to check Array.isArray(), so until we figure that out it might be irrelevant making changes in case we delete it in the end.

@dfdeagle47
Copy link

@Chalarangelo : ok makes sense now, I didn't know that part of the discussion.

@lock
Copy link

lock bot commented Dec 18, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for any follow-up tasks.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants