Avoiding nested callbacks when using MongooseJS

Question

I'm diving into the world of nodejs and mongo. I am playing around with a simple app that uses MongooseJS to talk to Mongo. I have been looking at examples and some GitHub projects for references, but the pattern seems very sloppy to me. Is this approach considered normal, or is there a better way?

Here is a sample from the app I am building with my daughter. Those nested callbacks feel like an anti-pattern to me. Am I wrong? Are there better ways to do it?

//userController.js when a new user registers an account is made and a document is cloned that will be their new "pet";

var Pet = require("models/pet.js");
var User = require("models/user.js");

exports.register = function (req, res, next) {
    var user = new User({
        username: req.body.username,
        password: req.body.password
    });

    user.save(function (err) {
        if (err) {
            next(err);
            return;
        }

        Pet.findOne({
            "name": "Egg",
            "ownerId": "<<system>>"
        }, function (err, pet) {
            if (err){
                next(err);
                return;
            }
            Pet.clonePetForUser(user._id, pet._id, function (err) {
                if (err) {
                    next(err);
                    return;
                }
                res.json({message: 'User created'});
            })
        }); 
    });
};

Answer 1

Yes you're right, there are better solutions. For example you could use promises instead of callbacks since mongoose operations can return promises according to the documentation.

Your example with promises:

var Pet = require("models/pet.js");
var User = require("models/user.js");

exports.register = function (req, res, next) {

    var user = new User({
        username: req.body.username,
        password: req.body.password
    });

    user.save()
        .then(function () {
            return Pet.findOne({
                "name": "Egg",
                "ownerId": "<<system>>"
            });
        })
        .then(function (pet) {
            return Pet.clonePetForUser(user._id, pet._id);
        })
        .then(function () {
            res.json({message: 'User created'});
        })
        .catch(function (err) {
            console.error(err);
            next(err);
        });

});

You can write your own method to return a promise. Instead of:

petSchema.statics.clonePetForUser = function (userId, petId, callback) {
     this.find({ }, callback);
};

You can write:

petSchema.statics.clonePetForUser = function (userId, petId) {
    return this.find({ });
};

Answer 2

This is due to asynchronous JavaScript, and this is called Callback Hell.

You can deal with it through some design patterns, the Zoltan's answer is a really good one (way better than mine). But I'll show you another here based on your original code.

---

Named callbacks

The main goal here is to reduce the indentations level to improve the code readability by splitting your script into named functions.

exports.register = function (req, res, next) {

    var user = new User({
        username: req.body.username,
        password: req.body.password
    });


    user.save(findAndClonePet);



    function findAndClonePet (err) {
        if (err) {
            next(err);
            return;
        }

        findPet();
    }



    function findPet () {
        Pet.findOne({
            "name": "Egg",
            "ownerId": "<<system>>"
        }, function (err, pet) {
            if (err){
                next(err);
                return;
            }
            clonePet(pet);
        });
    }



    function clonePet (pet) {
        Pet.clonePetForUser(user._id, pet._id, function (err) {
            if (err) {
                next(err);
                return;
            }
            res.json({message: 'User created'});
        });
    }



};

Hm ok it's better but at this point it's not "good" enough. We can see this part is not DRY :

if (err) {
    next(err);
    return;
}

Let's try this :

exports.register = function (req, res, next) {

    var user = new User({
        username: req.body.username,
        password: req.body.password
    });



    user.save(handleResponse(findAndClonePet));



    function findAndClonePet () {
        findPet();
    }



    function findPet () {
        Pet.findOne({
            "name": "Egg",
            "ownerId": "<<system>>"
        }, handleResponse(function (err, pet) {
            clonePet(pet);
        }));
    }



    function clonePet (pet) {
        Pet.clonePetForUser(user._id, pet._id, handleResponse(function () {
            res.json({message: 'User created'});
        }));
    }



    function handleResponse (callback) {
        return function (err) {
            if (err) {
                next(err);
                return;
            }

            // Forward all arguments to the callback
            callback.apply(null, Array.from(arguments));
        }
    }


};

At this point, you can go deeper, getting out the functions outside the register() scope for reusability purpose : You can build a little home made library to use thoses functions in other scripts of your application.