reduce the runtime of pick, fixes #285

[brentpayne]

This is code fixes #285

It reduces the runtime of pick from O(object properties * selection properties) to O(selection properties) or simply from O(nm) to O(n)

Additional bonus, pick and pickBy are now implemented using map, curry and a helper function.

[vqvu]

One thing, much of the code base uses function statements when possible. Can you change your function expressions to statements? Just to be consistent.

Other than that, looks good to me.

[svozza]

And possibly curry objectOnly directly so the maps can be written:

...
return this.map(objectOnly(pickSelection));
...
return this.map(objectOnly(pickBySelection));
...

[brentpayne]

updated

[vqvu]

I should have mentioned this earlier, but can you add a test for inherited and non-enumerable properties? Make sure we catch all of them.

[brentpayne]

good call, pickBy will only select enumerable keys. pick will select enumerable or non-enumerable keys. The old behavior was to select only enumerable keys, but we stated we'd change this b/c other libraries do pick non-enumerable properties, except ramda.

Before I make the change, are we in agreement that pick and pickBy should select non-enumerables?

[vqvu]

pick should. Can we make it work for pickBy? If not, we can just put a note in the docs saying one works on non-enumerable and one doesn't.

[brentpayne]

I believe I can make it work for pickBy as well. Expect an update tonight or tomorrow.

[brentpayne]

update pickBy to select enumerable or non-enumerable member variables on the object or from any associated prototypes. Updated tests to account for this. Specifically tests that failed because objects have non-enumerable function member variables starting with the '_' character.

[vqvu]

Function expression vs function statement again.

[brentpayne]

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.

[vqvu]

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?

[brentpayne]

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

[brentpayne]

done

[brentpayne]

All changes made, but I haven't run modified tests on an ES3 browser/engine.

[vqvu]

This would result in overridden properties being passed to f. Example

function f(p, v) {
    return v === 1;
}

var obj1 = {v: 1};
var obj2 = Object.create(obj1);
obj2.v = 2;

_([obj2]).pickBy(f).toArray(_.log) // => [{v: 1}]

In the reverse, it'll also disallow picking on default properties of objects, like hasOwnProperty, but that's not as important.

[vqvu]

Can you also add a test for the case I mentioned above? Call it "pickBy - overridden properties" or something.

[brentpayne]

subtle, but yes.
I'll fix it so overridden properties never make it to f

[brentpayne]

I added the test, but reverted my fix when I realized that it didn't originally fail. The reason why it doesn't fail is that the property may get tested twice, but always with the value relative to the object and not the prototype.
This is because testAndAdd is referrencing from the original object, obj2 here, rather than the prototype object, obj1. So v would always be 2.

I'm going to go ahead and prevent it from testing a property multiple times.

[vqvu]

Do you think seen should be Object.create(null) so that properties from Object.prototype get tested?

[brentpayne]

good point

[brentpayne]

fixed, added test the cover that case

[svozza]

Doesn't Object.create fall prey to same problem as Object.getPrototypeOf in that IE8 doesn't support it?

[vqvu]

What's the best way to deal with this? Put the Object.create behind a if (es5) check? Use strict seen[prop] === true instead of prop in seen? None of the Object.prototype properties === true, right?

[svozza]

The best way to deal with it is to drop support for IE8. ;) Yeah, I guess we could do

var seen = isES5 ? Object.create(null) : {}

And then the strict equals test (that's very nice, I never knew about that before).

[vqvu]

Cool. Fixed in 73a8cfe, though I don't have IE8 lying around, so I can't confirm.

[vqvu]

Looks good. Thanks for the great work!