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

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

Closed
wants to merge 1 commit into from

Conversation

megawac
Copy link
Collaborator

@megawac megawac commented May 20, 2014

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

if (_.isFunction(iterator)) {
iterator = createCallback(iterator, context);
} else {
var keys = _.invert(concat.apply([], slice.call(arguments, 1)));
Copy link
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
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
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
Copy link
Collaborator Author

megawac commented May 20, 2014

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

jdalton commented May 20, 2014

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

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