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

use forIn or forEach instead of map #51

Closed
gabrieldavid98 opened this issue Nov 23, 2017 · 7 comments
Closed

use forIn or forEach instead of map #51

gabrieldavid98 opened this issue Nov 23, 2017 · 7 comments

Comments

@gabrieldavid98
Copy link

Hi guys.

I was checking source code and i found that "map" function is used incorrectly. For that case i advise to use "forIn" or "forEach" instead.

Lines where i found that:

arr.map((value, key) => {

aKeys.map(val => {

Thanks for this library and for your job.
Greetings ✋

@selfrefactor
Copy link
Owner

Hi @GhostThrone and thanks for bringing this issue up.

Can you collaborate a bit further?

What is the benefit of forIn and forEach over map?

Also what you mean by map is used incorrectly as I fail to see that?

@MarcoWorms
Copy link

I think what he meant is that this functions mutate something out of their scope, and you aren't using the map to actually map and return new values, you are using it to iterate through an array to mutate something. IMO forEach is more semantic in this case because it clearly states you want to iterate this array to mutate something, but i wouldn't say this is a "wrong usage of map", you could test the performance to see if it makes a difference, if it doesn't than it's just a minor semantic issue :)

@selfrefactor
Copy link
Owner

@MarcoWorms Thanks for the clarification. It makes sense. I will do the benchmark and see if a change is required or not.

@selfrefactor
Copy link
Owner

Benchmark showed that forEach is good to go, so it replaces map in R.equals

In R.lastIndexOf I need both key and value so forEach is not applicable there.

The change is published in the new version 1.0.5 and I am closing the issue.

@MarcoWorms
Copy link

forEach also has access to key and value

@selfrefactor
Copy link
Owner

Right, right :)

I never use the method and I checked MDN docs for it and I read only the example where key is not showed.

My mistake - next version will address changing lastIndexOf and other methods where forEach can be used instead of map

@gabrieldavid98
Copy link
Author

gabrieldavid98 commented Nov 30, 2017

@MarcoWorms Thanks.
@selfrefactor I am so sorry, i didn't have time to reply before. Maybe i didn't explain in a good way the differences between map and forEach my mistake.
Thanks for all
Greetings ✋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants