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

Combine _.min and _.max #2142

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jridgewell
Copy link
Collaborator

http://jsperf.com/min-max-code-sharing

There's a hit on arrays without an iteratee. Looking into speeding that up, but everything else is faster.

@jashkenas
Copy link
Owner

We really gotta fix those create* names...

@jridgewell
Copy link
Collaborator Author

We really gotta fix those create* names...

It'd probably be good. Want to do that afterwards?

@jridgewell
Copy link
Collaborator Author

Hmm, I can get speeds to roughly the same levels, but brings back some duplication.

max (master, array, no iteratee) x 5,666,595 ops/sec ±1.46% (89 runs sampled)
max (combine, array, no iteratee) x 5,237,906 ops/sec ±2.17% (86 runs sampled)
max (master, array, iteratee) x 5,631,853 ops/sec ±1.51% (88 runs sampled)
max (combine, array, iteratee) x 5,169,098 ops/sec ±1.82% (83 runs sampled)
max (master, obj, no iteratee) x 342,269 ops/sec ±1.55% (82 runs sampled)
max (combine, obj, no iteratee) x 358,430 ops/sec ±1.95% (82 runs sampled)
max (master, obj, iteratee) x 324,928 ops/sec ±3.26% (82 runs sampled)
max (combine, obj, iteratee) x 361,950 ops/sec ±1.87% (86 runs sampled)
Fastest is max (master, array, no iteratee),max (master, array, iteratee)
var createExtremumFinder = function(dir) {
  return function(obj, iteratee, context) {
    var keys = !isArrayLike(obj) && _.keys(obj),
        length = (keys || obj).length,
        result = dir * Infinity,
        lastComputed = result,
        index = 0, currentKey, value;
    if (iteratee == null) {
      for (; index < length; index++) {
        currentKey = keys ? keys[index] : index;
        value = obj[currentKey];
        if (dir < 0 ? result > value : result < value) {
          result = value;
        }
      }
    } else {
      for (; index < length; index++) {
        currentKey = keys ? keys[index] : index;
        value = obj[currentKey];
        var computed = iteratee(value, currentKey, obj);
        if ((dir < 0 ? computed > lastComputed : computed < lastComputed) || index === 0 && computed === lastComputed) {
          result = value;
          lastComputed = computed;
        }
      }
    }
    return result;
  };
};

@jridgewell jridgewell force-pushed the combine-min-max branch 3 times, most recently from 3158197 to 80a26bb Compare April 4, 2015 00:07
var value = obj[currentKey];
var computed = iteratee ? iteratee(value, currentKey, obj) : value;
if ((max ? computed > lastComputed : computed < lastComputed) || (!found && computed === result)) {
found = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the found variable necessary? Can't you just infer it based on index

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, the iteratee can return anything. So, if the first iteration returns undefined, that won't be greater than -Infinity. But, the next iteration might be equal.

@megawac
Copy link
Collaborator

megawac commented Apr 9, 2015

Its faster for the iteratee (which are most common from my experience) and all object cases so thats good enough for me 👍

@jashkenas
Copy link
Owner

Rebase for a clean merge?

@jridgewell
Copy link
Collaborator Author

Edit: ha, wrong PR.

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.

None yet

4 participants