Skip to content

Abstract _.pick and _.omit via pick wrapper - #1639

Closed
megawac wants to merge 1 commit into
jashkenas:masterfrom
megawac:_.pick
Closed

megawac wants to merge 1 commit into
jashkenas:masterfrom
megawac:_.pick

Conversation

@megawac

@megawac megawac commented May 20, 2014

Copy link
Copy Markdown
Collaborator

Use a function generator to create _.pick and _.omit as they're essentially the same function.

We can expect a major performance improvement from this changeset for all cases of _.omit with some perf hit on the properties as argument case for _.pick

Comment thread underscore.js Outdated

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Let me know if the old way is preferred

Old way:

keys = _.map(concat.apply([], slice.call(arguments, 1)), String);
iterator = function(value, key) { return !_.contains(keys, key); };

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

http://jsperf.com/invert-vs-map which should be used for this case?

@jashkenas

Copy link
Copy Markdown
Owner

We can expect a major performance improvement from this changeset for all cases of _.omit with some perf hit on the properties as argument case for _.pick

Feel like showing it in a little JSPerf?

Use a pickerCreator function to create omit and pick
@megawac

megawac commented May 20, 2014

Copy link
Copy Markdown
Collaborator Author

I updated the pr from the original commit to be faster for the arguments case (slightly less terse now).

http://jsperf.com/underscore-1639/3

@jdalton

jdalton commented May 20, 2014

Copy link
Copy Markdown
Contributor

This looks like a mixed bag across browsers. I don't see a clear win for this change.

@braddunbar

Copy link
Copy Markdown
Collaborator

...not to mention the added complexity. I rather prefer them separate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants