3
\$\begingroup\$

I search for rails-like way of writing NodeJS code and it's more then hard to find the right way. The code quickly become bloated and unreadable. I would like my code to be as clean as possible.

Example (common ExpressJS middleware API controller):

var User = require('../models/user'); // mongoose model

exports.updateUser = function(req, res, next) {
  User.passwordToSecretWithBcrypt(req.body.password, function(err, secret) {
    if (err) return next(err);

    User.findById(req.user.id).exec(function(err, user) {
      if (err) return next(err);
      if (!user) return next(new Error('no user found'));

      if (req.body.name) user.name = req.body.name;
      if (req.body.email) user.email = req.body.email;
      if (secret) user.secret = secret;
      user.save(function(err, user) {
        if (err) return next(err);
        if (!user) return next(new Error('user not saved'));

        res.json(user);
      });
    });
  });
};

The main question is the error handling. A simple record update in the example above has 5 error handling statements. That's crazy in my opinion and you never know exactly what kind of error you'll get so it's very hard to construct the JSON error response that is shown to a user.

Is there a better way for handling errors?

I also tried to use promises but I couldn't find a best-practice example that would show me the whole example as a chain of the find, save, create and delete Mongoose methods.

How can I convert the example above to use promises?

Would promises fix the error handling? If so, how (question1)?

\$\endgroup\$
1
\$\begingroup\$

Interesting question,

I will not comment on making this more rails-like, I have no clue there. But I can observe that you in essence do 3 things: so you could perhaps group your error handling into 3 locations.

Furthermore, I want to point out that your code handles some error situations silently, that's never a good idea.

Other than that the code is very straightforward and easy enough to follow.

For my counter proposal I would start with a function that takes a request and checks for mandatory fields:

function validateMandatoryRequestFields( req , fields ){
  for( var i = 0 ; i < fields.length ; i++){
    if( !req.body[fields[i]] ){
      return fields[i] + ' was not provided';
    }
  }
  return '';
}

So that your function can do something like this in the beginning:

exports.updateUser = function(req, res, next) {
  //Do we have all we need ?
  var mandatoryFields = [ 'password', 'name' , 'email' ],
    errorMessage = validateMandatoryRequestFields( req , mandatoryFields );
  //Do we have what we need ?
  if(errorMessage){
    return next(errorMessage);
  }

You should always make sure that the request has what you need upfront.

Then, for callbacks I tend to mangle the err handling and bogus data handling into 1 if statement with 1 return statement:

  //Encrypt password
  User.passwordToSecretWithBcrypt(req.body.password, function(err, secret) {
    if (err || !secret ) {
      return next(err || new Error('Password encryption failed') );
    }

If you keep applying this to all of the code, you end up with something like this:

function validateMandatoryRequestFields( req , fields ){
  for( var i = 0 ; i < fields.length ; i++){
    if( !req.body[fields[i]] ){
      return fields[i] + ' was not provided';
    }
  }
  return '';
}    

exports.updateUser = function(req, res, next) {
  //Do we have all we need ?
  var mandatoryFields = [ 'password', 'name' , 'email' ],
    errorMessage = validateMandatoryRequestFields( req , mandatoryFields );
  //Do we have what we need ?
  if(errorMessage){
    return next(errorMessage);
  }
  //Encrypt password
  User.passwordToSecretWithBcrypt(req.body.password, function(err, secret) {
    if (err || !secret ) {
      return next(err || new Error('Password encryption failed') );
    }
    //Find user
    User.findById(req.user.id).exec(function(err, user) {
      if (err || !user){
        return next(err || new Error('no user found'));
      }
      //Update user
      user.save(function(err, user) {
        if (err || !user) {
          return next(err || new Error('user not saved') );
        }
        res.json(user);
      });
    });
  });
};
\$\endgroup\$
  • \$\begingroup\$ Thanks @konijn. That's still a looot of code for each REST API controller. Btw, I usually leave mongoose to handle all validation errors (including required fields). How about optimizing the code using promises? \$\endgroup\$ – xpepermint Oct 9 '14 at 20:49
  • \$\begingroup\$ I don't think handling both err and !user the same is the good idea. The former indicates a server-side problem like a coding bug or network failure accessing the database, while the latter could indicate a problem with the input-- the user provided to look-up was wrong or outdated. In the former case, you want some logging or notifications that the developers can see. In the latter case, simply returning an error to the client is appropriate. \$\endgroup\$ – Mark Stosberg Jan 20 '15 at 14:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.