I consider myself a very experienced node.js developer.

Yet I still wonder if there is a better way to write the following code so I don't get the pyramid of doom... Now I went easy on you, I have some code that my pyramid gets as high as 20 floors, no kidding; and that's WITH using async.js !!!

The problem is really that I have many dependencies on previews variables so everything must be nested. The guy that wrote the book "Async Javascript, build more responsive Apps with less code" explains that he would put the functions at the root scope, which sure, would get rid of the pyramid, but now you would have a whole bunch of high scope variables (possibly even global, depending at the scope you declare them at) and this pollution can result in some pretty nasty bugs (this could cause var conflicts with other scripts if set at global space (sure you could use self invoking functions, more yachhh... or even worse, since we are dealing with async, variable overrides...). In fact, the beauty of closure is pretty mush out the door.

What he remend is doing something like:

function checkPassword(username, passwordGuess, callback) {
    var passwordHash;
    var queryStr = 'SELECT * FROM user WHERE username = ?';
    db.query(selectUser, username, queryCallback);
    function queryCallback(err, result) {
        if (err) throw err;
        passwordHash = result['password_hash'];
        hash(passwordGuess, hashCallback);
    }

    function hashCallback(passwordGuessHash) {
        callback(passwordHash === passwordGuessHash);
    }
}

again, not a clean approach IMHO.

So, if you look at my code (again, this is just a snippet, I get much bigger nests in other places) you will often see my code getting further and further apart from the left; and that's with using things like waterfall and async forEach...

here is a small example:

ms.async.eachSeries(arrWords, function (key, asyncCallback) {
    pg.connect(pgconn.dbserver('galaxy'), function (err, pgClient, pgCB) {
        statement = "SELECT * FROM localization_strings WHERE local_id = 10 AND string_key = '" + key[0] + "'";
        pgClient.query(statement, function (err, result) {
            if (pgconn.handleError(err, pgCB, pgClient)) return;
            // if key doesn't exist go ahead and insert it
            if (result.rows.length == 0) {
                statement = "SELECT nextval('last_resource_bundle_string_id')";
                pgClient.query(statement, function (err, result) {
                    if (pgconn.handleError(err, pgCB, pgClient)) return;
                    var insertIdOffset = parseInt(result.rows[0].nextval);
                    statement = "INSERT INTO localization_strings (resource_bundle_string_id, string_key, string_revision, string_text,modified_date,local_id, bundle_id) VALUES ";
                    statement += "  (" + insertIdOffset + ",'" + key[0] + "'," + 0 + ",'" + englishDictionary[key[0]] + "'," + 0 + ",10,20)";
                    ms.log(statement);
                    pgClient.query(statement, function (err, result) {
                        if (pgconn.handleError(err, pgCB, pgClient)) return;
                        pgCB();
                        asyncCallback();
                    });
                });
            }
            pgCB();
            asyncCallback();
        });
    });
});

On my deep scripts I counted over 25 closing parenthesis, CRAZY, and all while remembering where to call my last callBack so async continues to next iteration...

Is there a solution to this problem? Or it is just the natrure of the beast?

I consider myself a very experienced node.js developer.

Yet I still wonder if there is a better way to write the following code so I don't get the pyramid of doom... Now I went easy on you, I have some code that my pyramid gets as high as 20 floors, no kidding; and that's WITH using async.js !!!

The problem is really that I have many dependencies on previews variables so everything must be nested. The guy that wrote the book "Async Javascript, build more responsive Apps with less code" explains that he would put the functions at the root scope, which sure, would get rid of the pyramid, but now you would have a whole bunch of high scope variables (possibly even global, depending at the scope you declare them at) and this pollution can result in some pretty nasty bugs (this could cause var conflicts with other scripts if set at global space (sure you could use self invoking functions, more yachhh... or even worse, since we are dealing with async, variable overrides...). In fact, the beauty of closure is pretty mush out the door.

What he remend is doing something like:

function checkPassword(username, passwordGuess, callback) {
    var passwordHash;
    var queryStr = 'SELECT * FROM user WHERE username = ?';
    db.query(selectUser, username, queryCallback);
    function queryCallback(err, result) {
        if (err) throw err;
        passwordHash = result['password_hash'];
        hash(passwordGuess, hashCallback);
    }

    function hashCallback(passwordGuessHash) {
        callback(passwordHash === passwordGuessHash);
    }
}

again, not a clean approach IMHO.

So, if you look at my code (again, this is just a snippet, I get much bigger nests in other places) you will often see my code getting further and further apart from the left; and that's with using things like waterfall and async forEach...

here is a small example:

ms.async.eachSeries(arrWords, function (key, asyncCallback) {
    pg.connect(pgconn.dbserver('galaxy'), function (err, pgClient, pgCB) {
        statement = "SELECT * FROM localization_strings WHERE local_id = 10 AND string_key = '" + key[0] + "'";
        pgClient.query(statement, function (err, result) {
            if (pgconn.handleError(err, pgCB, pgClient)) return;
            // if key doesn't exist go ahead and insert it
            if (result.rows.length == 0) {
                statement = "SELECT nextval('last_resource_bundle_string_id')";
                pgClient.query(statement, function (err, result) {
                    if (pgconn.handleError(err, pgCB, pgClient)) return;
                    var insertIdOffset = parseInt(result.rows[0].nextval);
                    statement = "INSERT INTO localization_strings (resource_bundle_string_id, string_key, string_revision, string_text,modified_date,local_id, bundle_id) VALUES ";
                    statement += "  (" + insertIdOffset + ",'" + key[0] + "'," + 0 + ",'" + englishDictionary[key[0]] + "'," + 0 + ",10,20)";
                    ms.log(statement);
                    pgClient.query(statement, function (err, result) {
                        if (pgconn.handleError(err, pgCB, pgClient)) return;
                        pgCB();
                        asyncCallback();
                    });
                });
            }
            pgCB();
            asyncCallback();
        });
    });
});

On my deep scripts I counted over 25 closing parenthesis, CRAZY, and all while remembering where to call my last callBack so async continues to next iteration...

Is there a solution to this problem? Or it is just the natrure of the beast?

• javascript
• node.js
• async.js

Share Improve this question Follow edited Aug 29, 2014 at 20:51 Jordan Running 106k1818 gold badges188188 silver badges187187 bronze badges asked Aug 29, 2014 at 20:34 born2netborn2net 25k2222 gold badges7272 silver badges105105 bronze badges 5

• 2 Note that you shouldn't use passwordHash === passwordGuessHash to pare a hashed string with a password, as this is vulnerable to timing attacks. Your crypto hash library (bcrypt) should have a cryptographically-safe parison function you can use instead. – Stuart P. Bentley Commented Aug 29, 2014 at 20:46
• If you want to -estetically- avoid having a lot of nesting, you can use nimble to chain the calls. If you want to -stackframely- avoid having a lot of nesting, you can defer the next call using a timer, declaring each function. – Luis Masuelli Commented Aug 29, 2014 at 20:47
• 2 You can use a promise-based style using something like q. – tcooc Commented Aug 29, 2014 at 20:47
• There is no framework that I know of for node.js to get away from async... – born2net Commented Aug 29, 2014 at 20:50
• have you tried wait.for? github./luciotato/waitfor – Lucio M. Tato Commented Sep 4, 2014 at 21:34

Add a ment  | 

5 Answers 5

Sorted by: Reset to default 3

As Mithon said in his answer, promises can make this code much clearer and help to reduce duplication. Let's say that you create two wrapper functions that return promises, corresponding to the two database operations you're performing, connectToDb and queryDb. Then your code can be written as something like:

ms.async.eachSeries(arrWords, function (key, asyncCallback) {
  var stepState = {};
  connectToDb('galaxy').then(function(connection) {
    // Store the connection objects in stepState
    stepState.pgClient = connection.pgClient;
    stepState.pgCB = connection.pgCB;

    // Send our first query across the connection
    var statement = "SELECT * FROM localization_strings WHERE local_id = 10 AND string_key = '" + key[0] + "'";
    return queryDb(stepState.pgClient, statement);
  }).then(function (result) {
    // If the result is empty, we need to send another 2-query sequence
    if (result.rows.length == 0) {
       var statement = "SELECT nextval('last_resource_bundle_string_id')";
       return queryDb(stepState.pgClient, statement).then(function(result) {
         var insertIdOffset = parseInt(result.rows[0].nextval);
         var statement = "INSERT INTO localization_strings (resource_bundle_string_id, string_key, string_revision, string_text,modified_date,local_id, bundle_id) VALUES ";
         statement += "  (" + insertIdOffset + ",'" + key[0] + "'," + 0 + ",'" + englishDictionary[key[0]] + "'," + 0 + ",10,20)";
         ms.log(statement);
         return queryDb(stepState.pgClient, statement);
       });
     }
  }).then(function (result) {
    // Continue to the next step
    stepState.pgCB();
    asyncCallback();
  }).fail(function (error) {
    // Handle a database error from any operation in this step...
  });
});

It's still plex, but the plexity is more manageable. Adding a new database operation to every "step" no longer requires a new level of indentation. Also notice that all error handling is done in one place, rather than having to add an if (pgconn.handleError(...)) line every time you perform a database operation.

Update: As requested, here's how you might go about defining the two wrapper functions. I'll assume that you're using kriskowal/q as your promise library:

function connectToDb(dbName) {
  var deferred = Q.defer();
  pg.connect(pgconn.dbserver(dbName), function (err, pgClient, pgCB) {
    if (err) {
      deferred.reject(err)
    } else {
      deferred.resolve({pgClient: pgClient, pgCB: pgCB})
    }
  });
  return deferred.promise;
}

You can use this pattern to create a wrapper around any function that takes a single-use callback.

The queryDb is even more straightforward because its callback gives you either a single error value or a single result value, which means that you can use q's built-in makeNodeResolver utility method to resolve or reject the deferred:

function queryDb(pgClient, statement) {
  var deferred = Q.defer();
  pgClient.query(statement, deferred.makeNodeResolver());
  return deferred.promise;
}

For more information on promises, check out my book: Async JavaScript, published by PragProg.

The problem to this sort of thing is promises. If you haven't heard of them, I suggest reading up on kriskowal's q.

Now, I don't know if the db.query returns a promise or not. If it doesn't you might be able to find a db-wrapper that does or a different db library. If that is not an option, you may "promisify" the db-library you're using. See Howto use promises with Node, and especially the section "Wrapping a function that takes a Node-style callback".

Best of luck! :)

The simplest way to bat the async pyramid of hell is to segregate your async callbacks into smaller functions that you can place outside your main loop. Chances are you can at least break some of your callbacks into more maintainable functions that can be used elsewhere in your code base, but the question you're asking is a bit vague and can be solved in a large number of ways.

Also, you should consider what Stuart mentioned in his answer and try to bine some of your queries together. I'm more concerned that you have 20+ nested calls which would indicate something seriously erroneous in your callback structure so I'd look at your code first before anything else.

Consider rewriting your code to have less back-and-forth with the database. The rule of thumb I use to estimate an app's performance under heavy load is that every async call will add two seconds to the response (one for the request, and one for the reply).

For example, is there maybe a way you could offload this logic to the database? Or a way to "SELECT nextval('last_resource_bundle_string_id')" at the same time as you "SELECT * FROM localization_strings WHERE local_id = 10 AND string_key = '" + key[0] + "'" (perhaps a stored procedure)?

I break each level of the pyramid of doom into a function and chain them one to the other. I think it is a lot easier to follow. In the example above i'd do it as follows.

ms.async.eachSeries(arrWords, function (key, asyncCallback) {

  var pgCB;
  var pgClient;

  var connect = function () {
    pg.connect(pgconn.dbserver('galaxy'), function (err, _pgClient, _pgCB) {
      pgClient = _pgClient;
      pgCB = _pgCB;
      findKey();
    });
  };

  var findKey = function () {
    statement = "SELECT * FROM localization_strings WHERE local_id = 10 AND string_key = '" + key[0] + "'";
    pgClient.query(statement, function (err, result) {
      if (pgconn.handleError(err, pgCB, pgClient))
        return;
      // if key doesn't exist go ahead and insert it
      if (result.rows.length == 0) {
        getId();
        return;
      }
      pgCB(); 
      asyncCallback(); 
    });

  };

  var getId = function () {
    statement = "SELECT nextval('last_resource_bundle_string_id')";
    pgClient.query(statement, function (err, result) {
      if (pgconn.handleError(err, pgCB, pgClient))
        return;
      insertKey();
    });
  };

  var insertKey = function () {
    var insertIdOffset = parseInt(result.rows[0].nextval);
    statement = "INSERT INTO localization_strings (resource_bundle_string_id, string_key, string_revision, string_text,modified_date,local_id, bundle_id) VALUES ";
    statement += "  (" + insertIdOffset + ",'" + key[0] + "'," + 0 + ",'" + englishDictionary[key[0]] + "'," + 0 + ",10,20)";
    ms.log(statement);
    pgClient.query(statement, function (err, result) {
      if (pgconn.handleError(err, pgCB, pgClient))
        return;
      pgCB();
      asyncCallback();
    });
  };

  connect();

});