Simplify query checks in Node.js+mongoose

Question

I'm writing a really simple RESTful API using Node.js, restify and mongoose but the code is getting complex and long. This is caused by all the checks that I have to do every time I make a query. For example, the next code show the controller for the endpoint POST /users/:user1/follow/:user2.

exports.followUser = function(req, res, next) {
  User.findOne({ username: req.params.user1 }, function(err, user1) {
    if (err) {
      res.status(500);
      res.json({ type: false, data: 'Error occurred: ' + err });
    } else {
      if (user1) {
        User.findOne({ username: req.params.user2 }, function(err, user2) {
          if (err) {
            res.status(500);
            res.json({ type: false, data: 'Error occurred: ' + err });
          } else {
            if (user2) {
              user2.followers.push(user1);
              user2.save();
              res.json({
                type: true,
                data: 'User ' + req.params.user1 + ' is now following user ' + req.params.user2
              });
            } else {
              res.json({
                type: false,
                data: 'User ' + req.params.user2 + ' not found'
              });
            }
          }
        });
      } else {
        res.json({
          type: false,
          data: 'User ' + req.params.user1 + ' not found'
        });
      }
    }
  });
};

How can I simplify this code? As you can see I'm checking the result of each findOne to know if the users received exist and I'm also returning json answers for each error but this is tedious. Is there a way to automatize this?

Jason Cust · Answer 1 · 2015-04-12T15:30:36.197

1

You could use Mongoose's promises to clean it up. Something like:

exports.followUser = function(req, res, next) {
  User.findOne({
    username: req.params.user1
  }).exec().then(function(user1) {
    if (user1 === null) {
      throw new Error('User ' + req.params.user1 + 'not found');
    }

    return User.findOneAndUpdate({
      username: req.params.user2
    }, {$push: {followers: user1}}).exec();
  }).then(function(user2) {
    if (user2 === null) {
      throw new Error('User ' + req.params.user2 + ' not found');
    }

    res.json({
      type: true,
      data: 'User ' + req.params.user1 + ' is now following user ' + req.params.user2
    });
  }, function(err) {
    res.status(500);
    res.json({ type: false, data: 'Error occurred: ' + err });
  });
};

edited Apr 12 '15 at 15:30

answered Apr 12 '15 at 14:06

Jason Cust

10,743
2
33
45

1

Looks promising. I'll try it. Thank you. – David Moreno García Apr 12 '15 at 14:19
I'm getting this error while saving user1 into res.locals: 'Error occurred: TypeError: Cannot set property 'user1' of undefined'. Any idea? – David Moreno García Apr 12 '15 at 14:46
Apologies, `res.locals` is an Express specific object. Removed the ned for it by just using a `findOneAndUpdate`. Updated to reflect this. – Jason Cust Apr 12 '15 at 15:29

Robert Moskal · Answer 2 · 2015-04-14T11:41:57.613

Promises help make the code a bit more readable, I see two other ways to simplify your code. You'll shave off a few lines and perhaps gain some legibility by handling errors using the built in restify error types

And secondly you could just update user2 instead of fetching him/her and simplify your if statements. Plus you gain an extra error check that you're missing now on the save. I came up with something like this:

exports.followUser = function(req, res, next) {
    User.findOne({username: req.params.user1}, function (err, user1) {

        next.ifError(err);

        if (!user1) 
            return next(new restify.NotFoundError('User ' + req.params.user1 + ' not found'));


        User.update({username: req.params.user2}, {$push: {followers: user1}}, function (err, updated) {

            next.ifError(err);

            if(!updated)
                return next(new restify.NotFoundError('User ' + req.params.user2 + ' not found'));

            return res.send({
                type: true,
                data: 'User ' + req.params.user1 + ' is now following user ' + req.params.user2
            });
            next();

        });

    });
};

if you want to keep your convention of returning a 200 response with extra information when a user isn't found you could create a custom error class:

var restify = require('restify');
var util = require('util');

function MyError(message) {
  restify.RestError.call(this, {
    restCode: 'MyError',
    statusCode: 200,
    message: message,
    constructorOpt: MyError
  });
  this.name = 'MyError';
};
util.inherits(MyError, restify.RestError);

And then create a global handler for that error as per the docs so you can return the json formatted the way you want.

I like the part of update instead of fetch and save. Good point. But I don't understand error handling. When you call next(err) what is happening? where is that error handle? — David Moreno García, Apr 12 '15 at 14:50
Look in the doc link I sent upi. You create a handler with an extra error parameter, and register it after all your api routes. If can centralize the handling of certain types of errors. I will update. — Robert Moskal, Apr 12 '15 at 14:55
You are linking to Express docs. Is that compatible with restify? I'm not using Express at all. — David Moreno García, Apr 12 '15 at 14:56

Simplify query checks in Node.js+mongoose

2 Answers2