Node.js API route for POSTing a resource. Getting rid of callback pyramid

Question

I'm writing an API for simple GET and POST routes. The app is a shared to-do-list. Users and lists are stored separately. A list can belong to many users, and a user can have many lists.

// User
{
    "_id": "54435e1424afb4c412f388bc",
    "email": "[email protected]",
    "lists": [
        "54435eca24afb4c412f388bd",
        "54435f1a24afb4c412f388be",
        "54435f1f24afb4c412f388bf"
    ],
    "name": "Batman"
},


// List
{
    "name": "Weekend",
    "users": [
        "54435e1424afb4c412f388bc",
        "543931f2894684f416b7b272"
    ],
    "tasks": [
        "Beat the Joker",
        "Buy groceries"
    ],
    "_id": "54435eca24afb4c412f388bd"
}

The POST route adds a list to the database, and makes sure to also writes the list's ID to the affected users.

My first version was a very deep callback pyramid (WIP can be seen here http://pastebin.com/BfB0yHKg). I decided to use Q promises to get around this, and now my (still WIP) code looks like below. Lots of helpers, and the execution is at the bottom. I'd appreciate some feedback on the general approach and promise pattern, but details are welcome too.

Note 1: The seemingly unnecessary conversions of async functions to promise versions are meant to unify the patterns and throw everything in the promise chain. I'd love some feedback on that thought. Note 2: Helpers will get more general and pulled out of the route function eventually!

router.post('/', function(req, res) {
    console.log('Creating a new list');

    var pParseRequest = function() {
        var deferred = Q.defer();
        try {
            var list = JSON.parse(req.body.list);
            deferred.resolve(list);
        } catch (error) {
            var e = new Error(error);
            e.name = 'ParseError';
            deferred.reject(e);
        }
        return deferred.promise;
    };

    var pValidateList = function(list) {
        var deferred = Q.defer();
        if (!list.name || list.name === '') {
            var e = new Error('Name missing or empty');
            e.name = 'DataError';
            deferred.reject(e);
        }
        if (!list.users || list.users.length < 1) {
            var e = new Error('Users missing or empty')
            e.name = 'DataError';
            deferred.reject(e);
        }
        if (!list.tasks) {
            var e = new Error('Tasks missing')
            e.name = 'DataError';
            deferred.reject(e);
        }
        deferred.resolve(list);
        return deferred.promise;
    };

    var pConnectDB = function(collection) {
        var deferred = Q.defer();
        try {
            var coll = req.db.get(collection);
            deferred.resolve(coll);
        } catch (error) {
            var e = new Error(error);
            e.name = 'DBError';
            deferred.reject(e);
        }
        return deferred.promise;
    };

    var pInsert = function(item, collection) {
        var deferred = Q.defer();
        collection.insert(item, function(error, doc) {
            if (error) {
                var e = new Error(error);
                e.name = 'DBError';
                deferred.reject(e);
            } else {
                deferred.resolve(doc);
            }
        });
        return deferred.promise;
    };

    var pUpdateUsers = function(list, usersCollection) {
        var promises = [];
        list.users.forEach(function(v, i) {
            var deferred = Q.defer();
            promises.push(deferred.promise);
            usersCollection.find({_id: v}, function(error, docs) {
                if (error) {
                    var e = new Error(error);
                    e.name = 'DBError';
                    deferred.reject(new Error(e));
                } else if (docs.length !== 1) {
                    var e = new Error('Found no or too many users');
                    e.name = 'DBError';
                    deferred.reject(e);
                } else {
                    var lists = docs[0].lists;
                    lists.push(list._id);
                    Q.fcall(function() {
                        pUpdateUserLists(v, lists, usersCollection);
                    })
                    .then(function() {
                        deferred.resolve();
                    });
                }
            });
        });
        return Q.all(promises)
        .then(function(f) {
            return list;
        });
    };

    var pUpdateUserLists = function(user, lists, collection) {
        var deferred = Q.defer();
        collection.update({_id: user}, { $set: { 'lists': lists } }, function(error, docs) {
            if (error) {
                var e = new Error(error);
                e.name = 'DBError';
                deferred.reject(e);
            } else {
                deferred.resolve();
            }
        });
        return deferred.promise;
    };

    var getStatusCode = function(message) {
        switch (message) {
            case 'ParseError':
            case 'DataError':
                return 400;
                break;
            case 'DBError':
                return 500;
                break;
            default:
                return 500;
                break;
        }
    };

    // GO!

    pParseRequest()
    .then(function(list) {
        return pValidateList(list);
    })
    .then(function(list) {
        return pConnectDB('lists')
        .then(function(listsCollection) {
            return pInsert(list, listsCollection);
        })
        .then(function(insertedList) {
            return pConnectDB('users')
            .then(function(usersCollection) {
                return pUpdateUsers(insertedList, usersCollection);
            })
            .then(function(list) {
                console.log('Responding with list');
                res.jsonp(list);
                return list;
            });
        });
    })
    .fail(function(error) {
        console.error('Error in the promise chain', error);
        var code = getStatusCode(error.name);
        res.status(code).send(error.message || 'Something went wrong');
    })
    .done();

});

Answer 1

I think you may be using Promises where they aren't needed, from what I can see. Promises are really meant for asynchronous operations, but it looks like some of the things you're doing with them are not actually async - the functions parseRequest and validateList particularly, they look like they should just be simple functions without when/then. If you already has the response from a request, and you're just operating on the response, then you're not working with an asynchronous operation.

$.getJSON('http://api.com/api/user', function(user){
  // I have a user now - I can do anything with it.
  $('#users ul').append('<li>' + user.name + '</li>');
});

What you're doing in parseRequest though is creating a deferred for really no reason - the JSON.parse function is not asynchronous. That function should probably be written like this:

var pParseRequest = function() {
    var result;

    try {
        // Synchronous operation - an error would occur if this fails to parse
        result = JSON.parse(req.body.list);
    } catch (error) {
        result = new Error(error);
        e.name = 'ParseError';
    }

    return result;

};

This is probably true for some of the other functions, but I would have to see this in action in order to know for sure.

In addition to this, you can reduce some of the code in this file by taking advantage of JavaScript's variable hoisting - I put some examples of this throughout your code. (I deleted some lines that I didn't modify)

router.post('/', function(req, res) {
    console.log('Creating a new list');

    // Does this function really need to be asynchronous?
    // It seems like you're using a promise when you shouldn't be.
    var pParseRequest = function() {
        var deferred = Q.defer();
        try {
            var list = JSON.parse(req.body.list);
            deferred.resolve(list);
        } catch (error) {
            var e = new Error(error);
            e.name = 'ParseError';
            deferred.reject(e);
        }
        return deferred.promise;
    };

    // Does this function really need to be asynchronous?
    var pValidateList = function(list) {
        var deferred = Q.defer();

        /*
            Instead of doing e.name and deferred.reject over 
            and over again, use the variable "e" as a flag.
        */
        var e = null;

        // if list.name === '', !list.name will still be true
        if (!list.name) {
            e = new Error('Name missing or empty');
        }
        if (!list.users || list.users.length < 1) {
            e = new Error('Users missing or empty');
        }
        if (!list.tasks) {
            e = new Error('Tasks missing');
        }

        if(e){
            e.name = 'DataError';
            deferred.reject(e);
        } else {
            deferred.resolve(list);
        }

        return deferred.promise;
    };


    var pUpdateUsers = function(list, usersCollection) {
        var promises = [];
        list.users.forEach(function(v, i) {
            var deferred = Q.defer();
            promises.push(deferred.promise);
            usersCollection.find({_id: v}, function(error, docs) {
                /*
                    Because of hoisting, var e should just be defined once
                    at the top of this function scope. 
                */
                var e = null;

                if (error) {
                    e = new Error(error);
                    e.name = 'DBError';
                    deferred.reject(new Error(e));
                } else if (docs.length !== 1) {
                    e = new Error('Found no or too many users');
                    e.name = 'DBError';
                    deferred.reject(e);
                } else {
                    var lists = docs[0].lists;
                    lists.push(list._id);
                    Q.fcall(function() {
                        pUpdateUserLists(v, lists, usersCollection);
                    })
                    .then(function() {
                        deferred.resolve();
                    });
                }
            });
        });
        return Q.all(promises)
        .then(function(f) {
            return list;
        });
    };


});