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 map for cloning memoizer #190

Merged
merged 2 commits into from Jun 1, 2016
Merged

Use map for cloning memoizer #190

merged 2 commits into from Jun 1, 2016

Conversation

@Marsup
Copy link
Member

Marsup commented May 31, 2016

Consistent 11% improvement over old version. It's not much but since clone is so heavily used...

const lookup = seen.orig.indexOf(obj);
if (lookup !== -1) {
return seen.copy[lookup];
const lookup = seen.has(obj);

This comment has been minimized.

Copy link
@arb

arb May 31, 2016

Contributor

Could you do seen.get(obj) here and then just return that if it's truthy rather than checking for it first, and then doing the .get?

This comment has been minimized.

Copy link
@Marsup

Marsup May 31, 2016

Author Member

I was worried something would be falsy in the future, javascript being what it is, but we'll see.

This comment has been minimized.

Copy link
@nlf

nlf Jun 1, 2016

Member

if javascript starts making empty objects falsey we're all in trouble

if (lookup !== -1) {
return seen.copy[lookup];
const lookup = seen.get(obj);
if (lookup) {

This comment has been minimized.

Copy link
@DavidTPate

DavidTPate Jun 1, 2016

Would this check be more appropriate to be if (lookup === undefined) {? According to the docs when the value isn't present undefined is returned.

That said I'm not sure what edge case could cause an item to return a falsey value when it is found from the map in this case.

This comment has been minimized.

Copy link
@nlf

nlf Jun 1, 2016

Member

nah, the check at the top of the method doesn't allow you to attempt to clone something that could be falsey. should be fine just like this.

@DavidTPate

This comment has been minimized.

Copy link

DavidTPate commented Jun 1, 2016

LGTM

@nlf nlf added the feature label Jun 1, 2016
@nlf nlf self-assigned this Jun 1, 2016
@nlf nlf added this to the 4.0.1 milestone Jun 1, 2016
@nlf

This comment has been minimized.

Copy link
Member

nlf commented Jun 1, 2016

👍 i like it. makes the code much simpler to read too

@nlf nlf merged commit db2a6ec into master Jun 1, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@nlf nlf deleted the clone-map branch Jun 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.