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

refactor: array loop to Array.forEach #19

Merged
merged 2 commits into from
Oct 19, 2019
Merged

refactor: array loop to Array.forEach #19

merged 2 commits into from
Oct 19, 2019

Conversation

curbengh
Copy link
Contributor

No description provided.

@curbengh curbengh requested a review from segayuu July 30, 2019 10:38
@coveralls
Copy link

coveralls commented Jul 30, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling b58a674 on array-map into 64f8583 on master.

@curbengh
Copy link
Contributor Author

initially, I wanted to

const result = languages.map(...)

but I don't know exactly how to return object { [key]: data[lang][key] }.

@segayuu
Copy link
Contributor

segayuu commented Jul 30, 2019

The following is my own opinion.
Array.prototype.forEach() looks beautiful because it eliminates some of the temporary variables, but I do not think that it will have a very good effect except for it.
When trying to break a loop, Array#forEach() has no way to do it (except to throw an exception) and needs to rewrite the whole loop.
Also, compared with Array#map() and Array#filter(), it does not make clearer intention than for loop.
If I conclude that I like it, I can not merge this PR.

@segayuu
Copy link
Contributor

segayuu commented Jul 30, 2019

I know that we can assign from an array to an object by using Array#reduce().
I like Array#reduce(), but like Array#forEach(), can write approximate content, so some people may not like it.

@curbengh
Copy link
Contributor Author

curbengh commented Jul 31, 2019

I see. forEach() cannot do if (!langData) continue;. What about the refactor in line 63? It doesn't seem have a need to break a loop.

btw, the first case doesn't need to break a loop, only skip it, so I can add a check,

  languages.forEach(lang => {
    Object.keys(data[lang]).forEach(key => {
      if (data[lang] && !Object.prototype.hasOwnProperty.call(result, key)) {
        result[key] = data[lang][key];
      }
    });

wouldn't if (data[lang]) {} have the same effect as if (!langData) continue;?

@segayuu
Copy link
Contributor

segayuu commented Jul 31, 2019

@curbengh Your opinion is correct.
In fact, the reason for not merging this PR is just my religious and coding guide.
I personally will not merge this PR, but I think that it may be merged upon review by other people :)

@segayuu segayuu requested review from a team and removed request for segayuu July 31, 2019 09:35
Copy link

@tcrowe tcrowe left a comment

Choose a reason for hiding this comment

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

👍

@curbengh
Copy link
Contributor Author

curbengh commented Aug 1, 2019

added another check.

@curbengh curbengh requested a review from tcrowe August 1, 2019 01:35
@NoahDragon NoahDragon merged commit 57e07ab into master Oct 19, 2019
@curbengh curbengh deleted the array-map branch October 19, 2019 03:40
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

Successfully merging this pull request may close these issues.

5 participants