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.

Sign up for GitHub

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

reduce the runtime of pick, fixes #285 #286

Merged
merged 12 commits into from
May 25, 2015
Merged

reduce the runtime of pick, fixes #285 #286

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
60ce252
reduce the runtime of pick from O(object properties * selection prope…
brentpayne Apr 25, 2015
f529f43
revert comment change
brentpayne Apr 25, 2015
bf7c349
using function(){} directly instead of setting as temporary variable.
brentpayne Apr 26, 2015
c5c7e01
made `pickBy` select from enumerable and non-enumerable members both …
brentpayne Apr 30, 2015
ac79f64
bug fix: props removed and check for hash key in output variable befo…
brentpayne May 22, 2015
34c63ee
moved isES5 constant to top of file, fall back to for ... in ... for …
brentpayne May 22, 2015
eefe423
changed comments to reflect inclusion of non-enumerables in ES5 selec…
brentpayne May 22, 2015
0bcee5c
Update index.js
brentpayne May 24, 2015
b7a233b
added overridden value test
brentpayne May 25, 2015
d304e53
prevent testing overriden properties multiple times
brentpayne May 25, 2015
2dca6ce
seen should be created from null, *Object.create(null)* . This avoid…
brentpayne May 25, 2015
ed98365
add test for fix in last commit
brentpayne May 25, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 39 additions & 29 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1693,6 +1693,23 @@ Stream.prototype.pluck = function (prop) {
};
exposeMethod('pluck');

/**
* Only applies the transformation strategy on Objects.
* This helper is used in `pick` and `pickBy`
**/

var objectOnly = _.curry(function(strategy, x) {
if (_.isObject(x)) {
return strategy(x);
}
else {
throw new Error(
'Expected Object, got ' + (typeof x)
);
}
});


/**
*
* Retrieves copies of all the enumerable elements in the collection
Expand Down Expand Up @@ -1723,32 +1740,23 @@ exposeMethod('pluck');
*/

Stream.prototype.pickBy = function (f) {

return this.consume(function (err, x, push, next) {
return this.map(objectOnly(function (x) {
var out = {};
if (err) {
push(err);
next();
}
else if (x === nil) {
push(err, x);
}
else if (_.isObject(x)) {
for (var k in x) {
if (f(k, x[k])) {
out[k] = x[k];
}
var props = []; // cache of already seen properties
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't look like anything is actually pushed to props. Also, instead of using an array and indexOf(which I assume is a linear operation), should we use a regular object instead (set props[prop] = true, check if (props[prop]))? We definitely don't want pickBy to be an O(n^2) operation.

Also, can you add a check for this case?

var proto = {a: 1};
var obj = Object.create(proto);
obj.a = 2;

_([obj]).pickBy(function (k, v) {
// check that only called with k === 'a' once.
}).resume();

Edit: http://jsperf.com/property-lookup-vs-indexof/2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great fix, was worried about this didn't think of this solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

var obj = x; // variable used to transverse prototype chain
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean "traverse" here?


// function created here so we don't create one per loop
var testAndAdd = function (prop) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Function expression vs function statement again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, it is an optimization b/c it creates the function once instead of once per loop iteration. I placed a comment above it to note why this code is inconsistent with the rest of the library.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what you mean. Couldn't you replace

var testAndAdd = function (prop) {
...
}

with

function testAndAdd(prop) {
...
}

Why would testAndAdd be created once per loop iteration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh, got it I thought you meant put the declaration inside the loop as an anonymous function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if (props.indexOf(prop) === -1 && f(prop, x[prop])) {
out[prop] = x[prop];
}
push(null, out);
next();
}
else {
push(new Error(
'Expected Object, got ' + (typeof x)
));
next();
}
});
};
do {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to check if obj == null and push an error. Highland should never throw an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is handled by objectOnly which throws an error if it is null or not an object

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, right. I didn't notice that.

Object.getOwnPropertyNames(obj).forEach(testAndAdd);
obj = Object.getPrototypeOf(obj);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Object.getPrototypeOf is not supported by IE8.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@brentpayne We have ES5 detection and the inherited keys function around the streamifyAll implementation. We should fallback to a for...in for ES3 (and mention this in the docs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks will switch this over

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done and noted in function doc string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added tests for ES3, but I don't have a headless ES3 browser for testing with testling. Any suggestions for testing this?

} while (obj);
return out;
}));
};
exposeMethod('pickBy');

Expand Down Expand Up @@ -1789,14 +1797,16 @@ exposeMethod('pickBy');
* });*/

Stream.prototype.pick = function (properties) {
return this.pickBy(function (key) {
return this.map(objectOnly(function(x) {
var out = {};
for (var i = 0, length = properties.length; i < length; i++) {
if (properties[i] === key) {
return true;
var p = properties[i];
if (p in x) {
out[p] = x[p];
}
}
return false;
});
return out;
}));
};
exposeMethod('pick');

Expand Down
81 changes: 73 additions & 8 deletions test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3369,13 +3369,42 @@ exports['pick - non-existant property'] = function (test) {
test.done();
};

exports['pick - non-enumerable properties'] = function (test) {
test.expect(5);
var aObj = {breed: 'labrador',
name: 'Rocky',
owner: 'Adrian',
color: 'chocolate'
}
Object.defineProperty(aObj, 'age', {enumerable:false, value:12});
delete aObj.owner;
aObj.name = undefined;

var a = _([
aObj // <- owner delete, name undefined, age non-enumerable
]);


a.pick(['breed', 'age', 'name', 'owner']).toArray(function (xs) {
test.equal(xs[0].breed, 'labrador');
test.equal(xs[0].age, 12);
test.ok(xs[0].hasOwnProperty('name'));
test.ok(typeof(xs[0].name) === 'undefined');
// neither owner nor color was selected
test.ok(Object.keys(xs[0]).length === 3);
});


test.done();
};

exports['pickBy'] = function (test) {
test.expect(4);

var objs = [{a: 1, _a: 2}, {a: 1, _c: 3}];

_(objs).pickBy(function (key) {
return key.indexOf('_') === 0;
_(objs).pickBy(function (key, value) {
return key.indexOf('_') === 0 && typeof value !== 'function';
}).toArray(function (xs) {
test.deepEqual(xs, [{_a: 2}, {_c: 3}]);
});
Expand Down Expand Up @@ -3408,8 +3437,8 @@ exports['pickBy'] = function (test) {

var objs4 = [Object.create({a: 1, _a: 2}), {a: 1, _c: 3}];

_(objs4).pickBy(function (key) {
return key.indexOf('_') === 0;
_(objs4).pickBy(function (key, value) {
return key.indexOf('_') === 0 && typeof value !== 'function';
}).toArray(function (xs) {
test.deepEqual(xs, [{_a: 2}, {_c: 3}]);
});
Expand All @@ -3424,8 +3453,8 @@ exports['pickBy - non-existant property'] = function (test) {

var objs = [{a: 1, b: 2}, {a: 1, d: 3}];

_(objs).pickBy(function (key) {
return key.indexOf('_') === 0;
_(objs).pickBy(function (key, value) {
return key.indexOf('_') === 0 && typeof value !== 'function';
}).toArray(function (xs) {
test.deepEqual(xs, [{}, {}]);
});
Expand All @@ -3443,15 +3472,51 @@ exports['pickBy - non-existant property'] = function (test) {

var objs3 = [{}, {}];

_(objs3).pickBy(function (key) {
return key.indexOf('_') === 0;
_(objs3).pickBy(function (key, value) {
return key.indexOf('_') === 0 && typeof value !== 'function';
}).toArray(function (xs) {
test.deepEqual(xs, [{}, {}]);
});

test.done();
};


exports['pickBy - non-enumerable properties'] = function (test) {
test.expect(5);
var aObj = {a: 5,
c: 5,
d: 10,
e: 10
}
Object.defineProperty(aObj, 'b', {enumerable:false, value:15});
delete aObj.c;
aObj.d = undefined;

var a = _([
aObj // <- c delete, d undefined, b non-enumerable but valid
]);


a.pickBy(function (key, value) {
if (key === 'b' || value === 5 || typeof value === 'undefined') {
return true
}
return false
}).toArray(function (xs) {
test.equal(xs[0].a, 5);
test.equal(xs[0].b, 15);
test.ok(xs[0].hasOwnProperty('d'));
test.ok(typeof(xs[0].d) === 'undefined');
// neither c nor e was selected, b is not selected by keys
test.ok(Object.keys(xs[0]).length === 3);
});


test.done();
};


exports['filter'] = function (test) {
test.expect(2);
function isEven(x) {
Expand Down