dns: Refactor forEach to map by benjamingr · Pull Request #5803 · nodejs/node · GitHub

Pull Request check-list

Affected core subsystem(s)

dns

Description of change

This is (as suggested) a more modular take on - #5762 , adding the changes one by one and making sure they're less objectionable. Making more smaller PRs.

Refactor a forEach to a map in the setServers function of the
dns module - simplifying the code. In addition, use more descriptive
variable names and const over var where possible.

Maybe I missed the memo but why is Array#map better than Array#forEach?

Aside, mixing several style changes in a single commit is kind of meh.

@bnoordhuis I tried to do as little unrelated changes as possible here and only edited one function.

Basically, the code was creating a new array, and then pushing to it in a loop and returning the results which is what Array#map already does. We were re-implementing map in the code.

I think map is easier to reason about in this case and it's more obvious what it does and why.

map is indeed easier to read here to me.

@ChALkeR care to review?

Does this always return a number?

Yes, it does. This is why I think r was confusing. It's SetServers in cares_wrap.cc which does:

  args.GetReturnValue().Set(err);

where err is either 0 for no error or a non-zero value for an error, where err is an int.

-1 to all the variable renaming. It makes this harder to review alongside the other changes.

Fair enough, I'll revert all the naming changes except for ipVersion and errorNumber is that acceptable @cjihrig or would you prefer I reverted those as well? I just think r and ver are really confusing variable names and I personally had a hard time making sure I understood what they meant when I've read them.

I think .map is also pre-allocating array where looping + pushing might require array resize internally.

Where did ver come from? ;-)

@ChALkeR I reverted most of the variable names as it seems people didn't like that change being included in this one. Sorry for the mess :)

This is inconsistent with if (ipVersion !== 0) in two places above.

@ChALkeR fixed. Thanks.

LGTM if the CI is happy.

CI: https://ci.nodejs.org/job/node-test-pull-request/1995/

@benjamingr ... looks like this needs a rebase and update.

Yeah, on top of my other (now landed) PR, I'll do it today and tomorrow and re-run CI, thanks :)

Unrelated newline? Is that accidental?

Yeah, that's because during the rebase I'll fix it.

LGTM

New CI: https://ci.nodejs.org/job/node-test-pull-request/2020/

Looks like there's a linting issue on this: https://ci.nodejs.org/job/node-test-linter/1770/console

Yeah, pushed a fix, any idea about the other issue? Is that just the build trolling?

Not sure, looks like an unrelated timeout. It's the first time I've seen it tho. /cc @Trott

You also pushed a build/config.gypi file with your changes

Refactor a forEach to a `map` in the `setServers` function of the dns module - simplifying the code. In addition, use more descriptive variable names and `const` over `var` where possible. PR-URL: nodejs#5803 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

Thanks, running a new CI https://ci.nodejs.org/job/node-test-pull-request/2024/

One failure in CI looks unrelated but can you please verify.

https://ci.nodejs.org/job/node-test-binary-arm/1451/RUN_SUBSET=5,nodes=pi1-raspbian-wheezy/console @jasnell looks unrelated, pinging @nodejs/build so they see the link.

Refactor a forEach to a `map` in the `setServers` function of the dns module - simplifying the code. In addition, use more descriptive variable names and `const` over `var` where possible. PR-URL: nodejs#5803 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

Landed in 4985c34 thanks!

Refactor a forEach to a `map` in the `setServers` function of the dns module - simplifying the code. In addition, use more descriptive variable names and `const` over `var` where possible. PR-URL: #5803 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

@benjamingr this patch does not land cleanly. Would you be able to backport it?

@thealphanerd I'm not sure we should backport it and in either case it's just a code cleanup. I can backport it though. I'll try to do a sweep through my PRs to master and do all the backporting to 5.x and 4.x next weekend.

thanks @benjamingr

benjamingr added the dns Issues and PRs related to the dns subsystem. label Mar 19, 2016

ChALkeR reviewed Mar 19, 2016
View reviewed changes

ChALkeR reviewed Mar 20, 2016
View reviewed changes

benjamingr force-pushed the dns-foreach-to-map branch from 384dfdc to 3747a13 Compare March 20, 2016 12:05

ChALkeR reviewed Mar 21, 2016
View reviewed changes

benjamingr force-pushed the dns-foreach-to-map branch from aa63331 to 1a96099 Compare March 22, 2016 09:41

benjamingr force-pushed the dns-foreach-to-map branch from 1a96099 to 568b586 Compare March 22, 2016 09:42

ChALkeR reviewed Mar 22, 2016
View reviewed changes

benjamingr force-pushed the dns-foreach-to-map branch from 568b586 to 797a272 Compare March 22, 2016 09:57

benjamingr mentioned this pull request Mar 22, 2016

dns: refactor dns to more modern JavaScript #5762

Closed

benjamingr force-pushed the dns-foreach-to-map branch from b40d64c to 669db03 Compare March 22, 2016 15:33

Trott mentioned this pull request Mar 22, 2016

Investigate flaky test-dgram-send-callback-buffer-length #5848

Closed

benjamingr closed this Mar 22, 2016

jasnell added the lts-watch-v4.x label Mar 22, 2016

evanlucas mentioned this pull request Mar 31, 2016

v5.10.0 Release Proposal #5970

Closed

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels May 3, 2016

dns: Refactor forEach to map by benjamingr · Pull Request #5803 · nodejs/node · GitHub

Uh oh!

Uh oh!

dns: Refactor forEach to map - #5803

dns: Refactor forEach to map#5803benjamingr wants to merge 1 commit intonodejs:masternodejs/node:masterfrom benjamingr:dns-foreach-to-mapbenjamingr/io.js:dns-foreach-to-mapCopy head branch name to clipboard

Conversation

benjamingr commented Mar 19, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

bnoordhuis commented Mar 19, 2016

Uh oh!

benjamingr commented Mar 19, 2016

Uh oh!

ChALkeR commented Mar 19, 2016

Uh oh!

benjamingr commented Mar 19, 2016

Uh oh!

ChALkeR Mar 19, 2016

Choose a reason for hiding this comment

Uh oh!

benjamingr Mar 20, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Mar 20, 2016

Uh oh!

benjamingr commented Mar 20, 2016

Uh oh!

YurySolovyov commented Mar 20, 2016

Uh oh!

ChALkeR Mar 20, 2016

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Mar 21, 2016

Uh oh!

ChALkeR Mar 21, 2016

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Mar 21, 2016

Uh oh!

cjihrig commented Mar 21, 2016

Uh oh!

jasnell commented Mar 21, 2016

Uh oh!

jasnell commented Mar 21, 2016

Uh oh!

benjamingr commented Mar 21, 2016

Uh oh!

ChALkeR Mar 22, 2016

Choose a reason for hiding this comment

Uh oh!

benjamingr Mar 22, 2016

Choose a reason for hiding this comment

Uh oh!

ChALkeR commented Mar 22, 2016

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

benjamingr commented Mar 22, 2016

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

targos commented Mar 22, 2016

Uh oh!

benjamingr commented Mar 22, 2016

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

benjamingr commented Mar 22, 2016

Uh oh!

benjamingr commented Mar 22, 2016

Uh oh!

MylesBorins commented Apr 8, 2016

Uh oh!

benjamingr commented Apr 10, 2016

Uh oh!

MylesBorins commented Apr 11, 2016

dns: Refactor forEach to map#5803
benjamingr wants to merge 1 commit into
nodejs:masterfrom
benjamingr:dns-foreach-to-map