Node.js Sequelize Promise

Question

I am working on sending a reset token via email to a potential user in Node.js, Sequelize, and Express.

The code works, but I still think it could be improved by handling the promises better.

exports.sendResetToken = function(req,res) {
  const username = req.headers.username;
  User.findOne({
    where: {username: username}
  }).then((user) => {
    if(user == null){
      res.sendStatus(404);
    } else {
      let token = user.generateResetToken();
      user.update({
        resetToken: token,
      }).then((user) => {
        email.sendResetToken(user.email,user.resetToken);
        res.sendStatus(200);
      }).catch((err) => {
        console.error(err);
        res.sendStatus(400);
      })
    }
    return null;
  }).catch((err) => {
    console.error(err);
    res.sendStatus(401);
  })
}

Answer 1

Reading your code I can make the following remarks:

1. I don't like the name User as a service as it looks like it specifies one User object. A better name would be Users or UsersApi or UserService.

2. Having a User service return null in case of the user not found should return a rejected promise instead of a fulfilled promise as it greatly simplifies the logic of your code. In that case you won't have to do this:

   if(user == null){
     res.sendStatus(404);
   }
   

3. You can wrap all the thenable code in functions so that your code will become more readable. For example this:

   .then((user) => {
       if(user == null){
         res.sendStatus(404);
       } else {
         let token = user.generateResetToken();
         user.update({
           resetToken: token,
         }).then((user) => {
           email.sendResetToken(user.email,user.resetToken);
           res.sendStatus(200);
         })
   

   will become this:

   .then(generateResetToken)
   .then(sendResetToken)
   

4. You do not have to return null as the intent of the function is to send an email here and not return anything. This also makes it more confusing.

5. Handle all your error cases in the catch function such as this:

   function handleErrors(error) {
      console.error(err);
      switch(error.reason) {
        case ERRORS.USER_NOT_FOUND:
           res.sendStatus(404);
           break;
        case ERRORS.USER_UPDATE_FAILURE:
           res.sendStatus(400);
           break;
        default:
           res.sendStatus(417); // Or more cases
      }
   }
   

So lets recap:

exports.sendResetToken = function(req,res) {
   const username = req.headers.username;

   Users
     .findOne({
        where: {username: username}
     }).then(generateResetToken)
     .then(sendResetToken)
     .catch(handleErrors)

    function generateResetToken(user) {
      const token = user.generateResetToken();
      const userUpdatePromise = user.update({
        resetToken: token,
      });

      return userUpdatePromise;
    } 

    function sendResetToken(user) {
      EmailService.sendResetToken(user.email,user.resetToken);
      res.sendStatus(200);
    }

    function handleErrors(error) {
      console.error(err);
      switch(error.reason) {
        case ERRORS.USER_NOT_FOUND:
          res.sendStatus(404);
        case ERRORS.USER_UPDATE_FAILURE:
          res.sendStatus(400);
        default:
          res.sendStatus(417) // Or more cases here
    }
  } 
}