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

dns: Refactor forEach to map #5803

Closed
wants to merge 1 commit into from

Conversation

benjamingr
Copy link
Member

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.

@benjamingr benjamingr added the dns Issues and PRs related to the dns subsystem. label Mar 19, 2016
@bnoordhuis
Copy link
Member

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
Copy link
Member Author

@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
Copy link
Member

ChALkeR commented Mar 19, 2016

map is indeed easier to read here to me.

@benjamingr
Copy link
Member Author

@ChALkeR care to review?


throw new Error('IP address is not properly formatted: ' + serv);
});

var r = cares.setServers(newSet);
const errorNumber = cares.setServers(newSet);
Copy link
Member

Choose a reason for hiding this comment

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

Does this always return a number?

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

cjihrig commented Mar 20, 2016

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

@benjamingr
Copy link
Member Author

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
Copy link

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


var match = serv.match(/\[(.*)\](:\d+)?/);
if (ipVersion !== 0)
return [ver, serv];
Copy link
Member

Choose a reason for hiding this comment

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

Where did ver come from? ;-)

@benjamingr
Copy link
Member Author

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


if (ver)
return newSet.push([ver, s]);
if (ipVersion)
Copy link
Member

Choose a reason for hiding this comment

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

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

@benjamingr
Copy link
Member Author

@ChALkeR fixed. Thanks.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 21, 2016

LGTM if the CI is happy.

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

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

@benjamingr
Copy link
Member Author

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

// reset the servers to the old servers, because ares probably unset them
cares.setServers(orig.join(','));

var err = cares.strerror(r);
throw new Error(`c-ares failed to set servers: "${err}" [${servers}]`);

Copy link
Member

Choose a reason for hiding this comment

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

Unrelated newline? Is that accidental?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@ChALkeR
Copy link
Member

ChALkeR commented Mar 22, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

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

@benjamingr
Copy link
Member Author

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

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

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

@targos
Copy link
Member

targos commented Mar 22, 2016

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>
@benjamingr
Copy link
Member Author

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

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

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

@benjamingr
Copy link
Member Author

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.

Trott pushed a commit to Trott/io.js that referenced this pull request Mar 22, 2016
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>
@benjamingr
Copy link
Member Author

Landed in 4985c34 thanks!

@benjamingr benjamingr closed this Mar 22, 2016
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
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>
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
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>
@MylesBorins
Copy link
Contributor

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

@benjamingr
Copy link
Member Author

@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
Copy link
Contributor

thanks @benjamingr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants