Javascript - Callback after all nested forEach loops are completed

Question

I'm sure this is a fairly simple task, but I'm not able to wrap my head around it at this time. I've got a nested set of forEach loops, and I need to have a callback for when all the loops are done running.

I'm open to using async.js

This is what I'm working with:

const scanFiles = function(accounts, cb) {
  let dirs = ['pending', 'done', 'failed'];
  let jobs = [];

  accounts.forEach(function(account) {
    dirs.forEach(function(dir) {
      fs.readdir(account + '/' + dir, function(err, files) {
         files.forEach(function(file) {
            //do something
            //add file to jobs array
            jobs.push(file);
         });
      });
    });
  });

  //return jobs array once all files have been added
  cb(jobs);
}

Rehnberg

Consider using glob. – Metallo

Is it ok to have scanFiles returning a Promise instead of working with callbacks? – Transceiver

Answer 1

Using forEach's 2nd parameter, the index, you can carry out a check whether all loops are done each time you run the innermost loop.

Thus with only a few lines added to your code you get this:

const scanFiles = function(accounts, cb) {
    let dirs = ['pending', 'done', 'failed'];
    let jobs = [];

    accounts.forEach(function(account, accIndex) {
        dirs.forEach(function(dir, dirIndex) {
            fs.readdir(account + '/' + dir, function(err, files) {
                files.forEach(function(file, fileIndex) {
                    //do something
                    //add file to jobs array
                    jobs.push(file);

                    // Check whether each loop is on its last iteration
                    const filesDone = fileIndex >= files.length - 1;
                    const dirsDone = dirIndex >= dirs.length - 1;
                    const accsDone = accIndex >= accounts.length - 1;

                    // all three need to be true before we can run the callback
                    if (filesDone && dirsDone && accsDone) {
                        cb(jobs);
                    }
                });
            });
        });
    });
}

Gastrula

suggesting an improvement: instead of checking greater than OR equal to. Just check equal to. const filesDone = fileIndex === files.length - 1;. Now, you're checking for one rather than two. – Kujawa

As I said in my edit rejection, it's best practice to check for greater than too. In case of an off by one error somewhere, which could give rise to an infinite loop. – Gastrula

great solution! Awarded you the correct answer for getting it done with pure JS. Thanks! – Rehnberg

Off by one? arrays are zero indexed. `let dirs = ['pending', 'done', 'failed']; // dirs.length = 3; The last index = 2; so last = 2 === dirs.length - 1; – Kujawa

@LuisEstevez again, strict equality checking will work perfectly well in this bit of code, but experience has shown me that it's always worth building fail-safes into your code; even when you don't think it's strictly necessary. Hence the >=. – Gastrula

All it takes is for an otherwise minor bug to increment fileIndex by 1 in the wrong place and suddenly you've got yourself an infinite loop. Using >= prevents a minor bug from becoming a major one. – Gastrula

There will be an error in the code if the index falls into the abyss. Also, unit test your production code and match the expected and making index in forEach immutable after each loop is learning from experienced. – Kujawa

I don't have a problem with being wrong. I'm trying to help you with what you believe which is that JS in some occasion has a problem computing this `3 === [0,1,2,3].length - 1; If I had a problem, I would have downvoted your answer which I didn't. – Kujawa

Answer 2

Simpler solution

No need for loops and pushing to arrays

I noticed that all of the answers here use a lot of complicated code. You can make it much simpler:

let fs = require('mz/fs');
let path = require('path');

let d = ['pending', 'done', 'failed'];
let a = ['A', 'B', 'C']; // <-- example accounts

let paths = [].concat.apply([], d.map(d => (a.map(a => path.join(d,a)))));
Promise.all(paths.map(path => fs.readFile(path, 'utf-8'))).then(files => {
  // you have all data here
}).catch(error => {
  // handle errors here
});

Explanation

If you use the promise version of fs - currently you can use:

let fs = require('mz/fs');

with the mz module:

• https://www.npmjs.com/package/mz

and soon it will be native in Node, see:

• https://github.com/nodejs/node/pull/5020

then you will be able to do things like the code below. Using the data:

// directories:
let d = ['pending', 'done', 'failed'];
// accounts:
let a = ['A', 'B', 'C'];

You can easily create an array of paths:

let paths = [].concat.apply([], d.map(d => (a.map(a => path.join(d,a)))));

From which you can create an array of promises:

let promises = paths.map(path => fs.readFile(path, 'utf-8'));

You can even use Promise.all() to have all of your files read:

let data = Promise.all(promises);

Now you can use everything as:

data.then(files => {
  // you have everything ready here
}).catch(error => {
  // some error happened
});

Note: you need to require two modules for the above code to work:

let fs = require('mz/fs');
let path = require('path');

Johnsiejohnson

I love your added your solution, using list comprehension is definitely less complicated. – Kujawa

This is great. I will need to study this to make full sense of the array operations. Cheers – Rehnberg

Answer 3

You can use walk

  walker.on("end", function () {
    console.log("all done");
    cb(jobs);
  });

Prickett

Answer 4

Simple counter

One simple way could be just to keep a counter.

const scanFiles = function(accounts, cb) {
  let dirs = ['pending', 'done', 'failed'];
  let jobs = [];

  // Variables to keep track of
  const lastAccountIndex = accounts.length * dirs.length;
  let indexCounter = 0;

  accounts.forEach(function(account) {
    dirs.forEach(function(dir) {  
      fs.readdir(account + '/' + dir, function(err, files) {
        files.forEach(function(file) {
          //do something
          //add file to jobs array
          jobs.push(file);

          indexCounter++;
        });

        //return jobs array once all files have been added
        if (lastAccountIndex === indexCounter) {
          cb(jobs);
        }
      });
    });
  }); 
}

Promise

Alternatively, fs + promise could be very useful here.

const scanFiles = function(accounts) {
  let dirs = ['pending', 'done', 'failed'];
  let jobs = [];

  const filePromises = []; 
  accounts.forEach(function(account) {
    dirs.forEach(function(dir) {
      filePromises.push(new Promise((resolve, reject) => {
        fs.readdir(account + '/' + dir, function(err, files) {
          files.forEach(function(file) {
            resolve(file);
          });
        });
      }));
    });
  });
  return Promise.all(filePromises);
}

scanFiles(someAccounts)
.then((files) => {
    files.forEach((file) => {
    // At this point, iwll the files will be scanned
    // So, do whatever you want with all the files here.
  });
});

fs-promise

Or just use https://www.npmjs.com/package/fs-promise

Kotta

Keeping a counter in this manner does work, but only in this specific case where each account has the same number of directories – Gastrula

Answer 5

If you use asyc library https://caolan.github.io/async/docs.html, your code will be much faster. (forEach is blocking [JavaScript, Node.js: is Array.forEach asynchronous?).

const scanFiles = function (accounts, cb) {
let dirs = ['pending', 'done', 'failed'];
let jobs = [];

async.each(accounts, function (account, accountCallback) {
    async.each(dirs, function (dir, dirCallback) {

        fs.readdir(account + '/' + dir, function (err, files) {
            if(err) console.log(err);

            async.each(files, function (file, fileCallback) {
                //do something
                //add file to jobs array
                jobs.push(file);
                fileCallback();

            }, dirCallback);

        });
    }, accountCallback);
}, function (err) {
    //return jobs array once all files have been added
    if (err) throw err;
    cb(jobs)
});

};

Gilbertegilbertian

Answer 6

So the problem is that you were sending an empty result before fs.readdir was done because nodeJS is async. So the solution is to add the callback inside the fs.readdir function.

const scanFiles = function (accounts, cb) {
    let dirs = ['pending', 'done', 'failed'];
    let jobs = [];

    accounts.forEach(function (account, i) {
        dirs.forEach(function (dir, j) {
            fs.readdir(account + '/' + dir, function (err, files) {
                files.forEach(function (file, k) {
                    //do something
                    //add file to jobs array
                    jobs.push(file);
                });
                if (i === accounts.length - 1 && j === dirs.length - 1 && k === files.length - 1) {
                    //return jobs array once all files have been added
                    cb(jobs);
                }
            });
        });
    });
}

Kujawa

This will run the callback many times; once for each dir in each account – Gastrula

You're correct, left out if statement to check it's last loop. – Kujawa