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'});
            })
        }); 
    });
};

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({ });
};

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.