4
\$\begingroup\$

Mind the code below:

var deltaArray = function(a,b){
    var deltaPtrs = [];
    var deltaVals = [];
    for (var i=0, l=a.length; i<l; ++i)
        if (a[i] !== b[i])
            deltaPtrs[deltaPtrs.length] = i,
            deltaVals[deltaVals.length] = b[i];
    return [deltaPtrs,deltaVals];
};

var deltaHash = function(a,b){
    var deltaKeys = [];
    var deltaVals = [];
    for (var key in b)
        if (a[key] !== b[key])
            deltaKeys[deltaKeys.length] = key,
            deltaVals[deltaVals.length] = b[key];
    return [deltaKeys,deltaVals];
};

That code is obviously duplicated. Considering that the length of the involved arrays/objects is huge (>1kk elements), and that adding an if inside the loop would be detrimental to performance, is there any way to avoid this duplication?

Edit: here is a JSPerf for your enjoyment.

\$\endgroup\$
  • \$\begingroup\$ Do you realize that your if statement needs braces so that it encompasses both of the statements after it? \$\endgroup\$ – jfriend00 Jun 5 '14 at 21:06
  • \$\begingroup\$ You can simplify some of those assignments to deltaPtrs.push(i) and the same for the other appends. \$\endgroup\$ – Greg Hewgill Jun 5 '14 at 21:08
  • 5
    \$\begingroup\$ @jfriend00: Actually, there's a comma , between those expressions, which means they are one statement. Personally, I would reject that construct in a code review for exactly the reason you state. \$\endgroup\$ – Greg Hewgill Jun 5 '14 at 21:10
  • \$\begingroup\$ Ah, sorry. I am aware that is unusual, but I think it is more elegant so I do defend it. Greg, you're right, but the performance downgrades. \$\endgroup\$ – MaiaVictor Jun 5 '14 at 21:44
  • \$\begingroup\$ @Dokkat push is generally faster; see this jsperf for instance. And if you want to avoid braces, write CoffeeScript instead: the comma-trick is a step too far, regardless of aesthetics. \$\endgroup\$ – Flambino Jun 6 '14 at 0:02
1
\$\begingroup\$

The simplest answer is to remove deltaArray and use deltaHash for everything. As long as your arrays don't have any extra properties, the results will be identical, except the keys are stored as strings instead of numbers.


I'm not sure it's useful to store the keys in one array and the values in another. Storing the keys in an array could be useful if ordering the keys is important, but that doesn't apply here (since we got the keys from a for...in loop, we encountered them in an undefined order). So, you could let the keys be keys and let the values be values, and if you need an array of keys, use Object.keys.

var deltaHash = function(a, b) {
    var delta = {};
    for (var key in b)
        if (a[key] !== b[key])
            delta[key] = b[key];
    return delta;
};

If you still want a separate function for arrays, it would look like this:

var deltaArray = function(a, b) {
    var delta = {};
    for (var i = 0, l = a.length; i < l; ++i)
        if (a[i] !== b[i])
            delta[i] = b[i];
    return delta;
};

You'd still probably want to eliminate redundancy in this case. We could extract the duplicated code into a separate function and end up with something like this:

var diff = function(a, b, delta, key){
    if (a[key] !== b[key])
        delta[key] = b[key];
};

var deltaHash = function(a, b) {
    var delta = {};
    for (var key in b)
        diff(a, b, delta, key);
    return delta;
};

var deltaArray = function(a, b) {
    var delta = {};
    for (var i = 0, l = a.length; i < l; ++i)
        diff(a, b, delta, i);
    return delta;
};

Somehow that's not very satisfying, though; it still feels redundant. What we have at this point really feels like it wants to be more of a classical design:

function Delta() { }

Delta.prototype.diff = function(a, b, key) {
    if (a[key] !== b[key])
        this[key] = b[key];
};

Delta.prototype.initFromArrays = function(a, b) {
    for (var i = 0, l = a.length; i < l; ++i)
        this.diff(a, b, i);
};

Delta.prototype.initFromObjects = function(a, b) {
    for (var key in b)
        this.diff(a, b, key);
};

But again, most of this is not necessary if you decide to build deltas for arrays the same way you build them for other objects, using something like your original deltaHash for both. Aside from some possible naming and stylistic issues, and the awkwardness of having the keys in a separate array, it should do the trick (although you might want to add a hasOwnProperty check, and use push).

Hope this gives you some ideas.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ I tried to stick with the coding style in your question for these examples, but you may want to consider not omitting braces and cleaning up whitespace before posting, otherwise you might get a lot of comments and reviews about style when it's not really what you want to focus on. \$\endgroup\$ – Dagg Jun 6 '14 at 4:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.