dns: Refactor forEach to map

[benjamingr]

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.

[bnoordhuis]

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.

[benjamingr]

@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.

[ChALkeR]

map is indeed easier to read here to me.

[benjamingr]

@ChALkeR care to review?

[ChALkeR]

Does this always return a number?

[benjamingr]

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.

[cjihrig]

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

[benjamingr]

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.

[YurySolovyov]

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

[ChALkeR]

Where did ver come from? ;-)

[benjamingr]

@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 :)

[ChALkeR]

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

[benjamingr]

@ChALkeR fixed. Thanks.

[cjihrig]

LGTM if the CI is happy.

[jasnell]

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

[jasnell]

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

[benjamingr]

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

[ChALkeR]

Unrelated newline? Is that accidental?

[benjamingr]

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

[ChALkeR]

LGTM

[jasnell]

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

[jasnell]

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

[benjamingr]

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

[jasnell]

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

[targos]

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

[benjamingr]

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

[jasnell]

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

[benjamingr]

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.

[benjamingr]

Landed in 4985c34 thanks!

[MylesBorins]

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

[benjamingr]

@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.

[MylesBorins]

thanks @benjamingr