Recursively reading a directory in node.js

Question

I made a function for recursively reading a directory and it just seems kind of long and complicated. If you could take a look and tell me what's good or bad about it, that would be great.

Reading the directory synchronously is not an option.

var dutils = require('./dutils');
module.exports = {
 getIndex: function(ignore, cb) {
 var root = this.root;
 var cbs = 1;
 var index = {};
 // recursive function for walking directory
 (function walk(dir) {
 fs.readdir(dir, function(err, files) {
 if (err) return cb(err);
 files.forEach(function(fileName) {
 // prepare
 var filePath = path.join(dir, fileName); // filePath with root
 var relFilePath = path.relative(root, filePath); // filePath without root
 // if the file is on our ignore list don't index it
 if (dutils.globmatch(filePath, ignore)) return;
 var stats = fs.statSync(filePath);
 var isDir = stats.isDirectory();
 index[relFilePath] = {
 isDir: isDir,
 mtime: stats.mtime
 };
 // if the file is a directory, walk it next
 if (isDir) {
 // count the number of callbacks that are running concurrently
 cbs++;
 walk(filePath);
 }
 });
 // our callback was completed
 cbs--;
 // if all callbacks were completed, we've completely walked our directory
 if (cbs === 0) cb(null, index);
 });
 })(root);
 }
}

• Prinzhorn: You should read about control flow libraries, for example async or promises, for example q.
• Macks: Thanks for your commment, @Prinzhorn. I use async a lot but I'm not quite sure how it can help me clean up this particular code. Which feature of async do you think I should be looking at to deal with recursive functions? Thank you for your help!
• Prinzhorn: What you are doing with your cbs counter is essentially async.eachSeries(files, ...)
• Macks: Ok, it looks like I can just dynamically add to the array, that's really cool. Thanks a lot!

Answer 1

My comments are inline starting with the //*. I also very much encourage you to look at async libraries such as Q.

var dutils = require('./dutils');
//* Why export an object with a single function? Why not just export the function itself?
module.exports = {
 //* I don't like intermixing method implementation with exports declaration because you tend to be 
 //* looking for very different things when looking at each. Hoisting to the rescure!
 //* module.exports = { getIndex: getIndex };
 //* ... later ...
 //* function getIndex(ignore, cb) { 
 //* ...
 getIndex: function(ignore, cb) {
 //* What if they didn't pass a callback? Might as well check here and throw an error. Otherwise
 //* there will be a confusing error later on
 //* What is `this`? I **highly** recommend not using the `this` parameter unless you 
 //* understand _very_ well what it does and how it works. You can almost always achieve the same
 //* thing using simpler techniques
 var root = this.root;
 //* cds is not a good variable name - I have no idea what this represents reading it
 var cbs = 1;
 var index = {};
 // recursive function for walking directory
 (function walk(dir) {
 //* I usually prefer naming callback functions. Not only does it allow you to give a good
 //* label to the code but the function will show up labeled in stack traces
 fs.readdir(dir, function(err, files) {
 //* Be aware that it is possible for the callback to be triggered multiple times when there are 
 //* errors. This is an unusual interface and just be certain that you do this on purpose
 if (err) return cb(err);
 //* Again, I would recommend labeling this function.
 //* Also I'm not sure if this will follow symlinks or not but if does then this might end up in
 //* an endless loop.
 files.forEach(function(fileName) {
 // prepare
 var filePath = path.join(dir, fileName); // filePath with root
 var relFilePath = path.relative(root, filePath); // filePath without root
 //* I don't think this is really a useful comment as it doesn't say much more than
 //* the code itself does. I also usually prefer putting the `return` statement for early returns
 //* on its own line to make the early return stand out visually.
 // if the file is on our ignore list don't index it
 if (dutils.globmatch(filePath, ignore)) return;
 var stats = fs.statSync(filePath);
 var isDir = stats.isDirectory();
 index[relFilePath] = {
 isDir: isDir,
 mtime: stats.mtime
 };
 //* This comment is not very useful as it - again - just restates what the code does
 // if the file is a directory, walk it next
 if (isDir) {
 // count the number of callbacks that are running concurrently
 cbs++;
 walk(filePath);
 }
 });
 // our callback was completed
 cbs--;
 // if all callbacks were completed, we've completely walked our directory
 if (cbs === 0) cb(null, index);
 //* Is the entire point of the `cbs` counter to see if any `readdir` operation resulted in an error?
 //* that seems to be all that it does. Why not just have a `var lastError = null` in `getIndex` and
 //* rather than checking for err do `if(lastError = err) return`. That way if `err` is truthy you have
 //* an error, otherwise you do not
 //*
 //* Also keep in mind that cb will be called multiple times but each time with the 
 //* same instance of the index object. That means that previously invoked callbacks will be
 //* having this object mutate underneat them. This might be exactly what you want but it is unuaual
 //* and should definitely be documented clearly. At the very least you want a big bold doc comment
 //* on this function
 });
 })(root);
 }
}

Here is an article I wrote on this parameters

• Macks: Thanks for taking the time to review my code. The function is part of a more complex object, it's just dumbed down for this post. Your article on this is really interesting, I'll definitely keep it in mind. cbs ("callbacks") is a counter for the number of asynchronous fs.readdir-calls. If it reaches 0, I know that I'm done. You bring up a good point with if (err) return cb(err);. I really didn't consider that cb could be invoked multiple times with an error AND possibly even with an index after that. What do you think an elegant solution to this problem would be? Thank you very much!
• George Mauer: Well I don't know what your desired behavior is. Do you want to bail out after the first error? In that case you should track each error as I suggested toward the end. Or perhaps you want all errors to be rolled up and returned at once or an exception to be thrown?
• George Mauer: By the way as for my notes about the code comments you'll find a lot of the underpinning philosophy in this excellent (and VERY long as always) Steve Yegge article
• Macks: Hey George, thanks for the quick reply! I want to bail out after the first error and actually @Prinzhorn's suggestion about using async.eachSeries solves this. My updated code following some of your suggestions now looks like this: snippi.com/s/mbdibu3 What do you think? Thank you very much for your help!

Answer 2

It's more efficient, better to maintain and the code becomes much cleaner if you use the promisify native Node function to transform the fs callback functions to promise based functions, then write your code with async/await style.

Code:

const fs = require('fs');
const { promisify } = require('util');
const path = require('path');
const readDirAsync = promisify(fs.readdir);
//
const sourcePath = process.argv[2] || './';
// 
(async () => {
 // Main execution 
 var finalRes = await scanDir(sourcePath);
 console.log('Scan result', finalRes);
 process.exit(0);
 // Recursive async function 
 async function scanDir(dir) {
 const list = await readDirAsync(dir);
 // Array.map returns promisses because the argument function is async
 return Promise.all(list.map(async (name) => {
 try {
 // File (or directory) info 
 const itemPath = path.join(dir, name);
 const stats = fs.statSync(itemPath);
 const isDir = stats.isDirectory();
 var children = null;
 // Recursion for directories 
 if (isDir) {
 children = await scanDir(itemPath);
 }
 //
 return {
 name,
 itemPath,
 isDir,
 parent: dir,
 children
 };
 }
 catch (err) {
 console.error('Error in scanDir()', err);
 }
 }));
 } 
})();

Scan output

• Mast: You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and why it is better than the original) so that the author and other readers can learn from your thought process.
• Lucas Ponzo: It's better than the other solution specially because it's not necessary to manage callback counts, also, the code is cleaner and easier to maintain and use the latest features of javascript/node.js. It's modern, but still very standard node javascript. The main points are already commented. The fact you need more comments to understand a code like this is not a valid reason to downvote it. I recommend this link for more information on the mindset needed to write neat code like this: medium.com/@tkssharma/…