openexchangerates / accounting.js

A lightweight JavaScript library for number, money and currency formatting - fully localisable, zero dependencies.
http://openexchangerates.github.io/accounting.js
MIT License
4.95k stars 532 forks source link

Array map #20

Closed millermedeiros closed 13 years ago

millermedeiros commented 13 years ago

changed some of the loops to Array.map().

wjcrowcroft commented 13 years ago

I was just checking out _.map in underscore.js:

  // Return the results of applying the iterator to each element.
  // Delegates to **ECMAScript 5**'s native `map` if available.
  _.map = function(obj, iterator, context) {
    var results = [];
    if (obj == null) return results;
    if (nativeMap && obj.map === nativeMap) return obj.map(iterator, context);
    each(obj, function(value, index, list) {
      results[results.length] = iterator.call(context, value, index, list);
    });
    return results;
  };

Compare to the new map method in accounting.js which achieves pretty much the same goal:

    function map(arr, fn, thisVal){
        var i = 0,
            n = arr.length,
            result = [];
        while(i < n){
            result[i] = fn.call(thisVal, arr[i], i, arr);
            i += 1;
        }
        return result;
    }

Maybe we can merge them - obviously underscore has its own each method to use, this one could stick to a fallback loop like above... Maybe something like this would work?:

    var nativeMap = Array.prototype.map; // earlier

    function map(obj, iterator, context){
        var results = [];
        if (obj == null) return results;

        // Use native .map method if it exists:
        if (nativeMap && obj.map === nativeMap) return obj.map(iterator, context);

        // Fallback for native .map:
        for (var i = 0; i < obj.length; i++ ) {
            results[i] = iterator.call(context, obj[i], i, obj);
        }
        return results;
    }

Just put that together in the comment and haven't actually tested it - I'll play around with it in the console now.. let me know if you see any issues.

millermedeiros commented 13 years ago

this will work and will be faster on modern browsers, didn't implemented like this for brevity, I would also cache the array length. see jsperf test

wjcrowcroft commented 13 years ago

Yeah, it looks great. Thanks for doing the jsperf test.

Want to commit the changes to this branch, so I can auto-merge it? Or I can merge these into dev and change it myself, if you don't have time, no problem.

wjcrowcroft commented 13 years ago

I've committed these to the dev branch, along with the changes we proposed. Thanks!