4

So I'm writing a function that makes a bunch of database calls. I want to store their results in an array and trigger a callback once they're all done.

Some pseudocode may help:

function getStuff (array, callback) {
    var results = [];
    var done = 0;
    for (var i = 0, len = array.length; i < len; i++) {
        database.fetchOne(array[i], function(result) {
            results[i] = result;
            done++;
            if (done == len)
                callback(results);
        });
    }
}

This works great. However, I'm told it's bad practice to nest a closure inside a loop because it keeps defining the function every iteration, and that comes at a performance cost.

Other answers suggest moving the callback outside of the loop:

function getStuff (array, callback) {
    var results = [];
    var done = 0;
    for (var i = 0, len = array.length; i < len; i++) {
        database.fetchOne(array[i], myCallback.bind(this, i, results, done, callback));
    }
}

function myCallback (i, results, done, callback, result) {
    results[i] = result;
    done++;
    if (done == len)
        callback(results);
}

But this doesn't work since done has an immutable type, so it doesn't change the value of done in getStuff.

So... what do I do?

Vale
  • 1,912
  • 16
  • 22
Cheezey
  • 700
  • 2
  • 11
  • 29
  • Just to clarify, `done` is not an immutable type, it's a primitive which is passed by value rather than by reference. It's a fine distinction, but one that I think is important to note. – Patrick Roberts Jul 12 '15 at 20:48
  • What context are you in? is this node.js? You could approach this by using promises and executing a callback when all are done, it should be a more elegant way of handling this. – Vale Jul 12 '15 at 20:49
  • 1
    @Sosdoc, it doesn't matter. In node.js and client-side JS alike, the same standards for asynchronous callbacks would apply. – Patrick Roberts Jul 12 '15 at 20:52
  • @PatrickRoberts I'm asking just for pointing out a possible solution using promises, in case this isn't node (but I doubt it) it could be done by just using jquery. – Vale Jul 12 '15 at 20:53
  • @Sosdoc I am in node. – Cheezey Jul 12 '15 at 20:59

2 Answers2

3

You can define the myCallback just once, instead of in every iteration.

function getStuff (array, callback) {
    var results = [];
    var done = 0;

    function myCallback(i, callback, result) { 
        // update results and done in here
    }
    for (var i = 0, len = array.length; i < len; i++) {
        database.fetchOne(array[i], myCallback.bind(this, i, results, done, callback));
    }
}
yts
  • 1,890
  • 1
  • 14
  • 24
  • 1
    I would personally globalize `results` to the closure as well rather than passing it to each function if I were to use this method. – Patrick Roberts Jul 12 '15 at 20:50
  • So how do I call `getStuff` when it's inside the anonymous function? – Cheezey Jul 12 '15 at 21:02
  • I just realized actually that would only allow `getStuff` to work once. Perhaps my edit wasn't so elegant. – Patrick Roberts Jul 12 '15 at 21:07
  • @PatrickRoberts Maybe if `done` and `results` were defined in the closure, but their values were set in `getStuff`? – Cheezey Jul 12 '15 at 21:07
  • @Cheezey that would be better but would still limit `getStuff` from being called in parallel since `done` and `results` would interfere with each other. – Patrick Roberts Jul 12 '15 at 21:09
  • 2
    maybe define myCallback, done and results inside getStuff, but not in the loop? – yts Jul 12 '15 at 21:11
  • @yts Yes that would be a good approach. Instead of being defined for each iteration it would only be defined once for each call to `getStuff`. Go ahead and edit it that way. – Patrick Roberts Jul 12 '15 at 21:13
  • @PatrickRoberts That should suffice for now; thank you both for your help! If any more efficient solutions are found, I'm still open to them. – Cheezey Jul 12 '15 at 21:16
2

Here's a solution using promises with Q

First of all, install Q with npm

npm install q

And remember to require it

var Q = require('q');

then your final code could be this

function getStuff (array, callback) {
    //denodeify transforms a node function into one that works with promises
    var fetch = Q.denodeify(database.fetchOne);
    // all waits for all promises to be resolved
    var promise = Q.all(array.map(fetch));

    // callback receives an array with all the return values from fetchOne
    promise.then(callback, function(error) {
        //this gets called in case any of the calls has an error
    });
}

This is a more elegant solution in my opinion, I recommend reading up on Q and all its possible usages, it can avoid nasty situations where you have lots of nested callback (often referred as 'callback hell')

Vale
  • 1,912
  • 16
  • 22