Handling login using promises

Question

We've got this node.js code which is called by an Express route:

routes.login = (req, res) => {

  let inpEmail = req.body.email.toLowerCase(),
    inpPassword = req.body.password,
    loginFailureCount = 0;

  // find the user
  User.findOne({
      email: inpEmail
    }).exec()
    .then((user) => {

      //get existing login failure count
      loginFailureCount = user.loginFailureCount;

      if (user.isLocked && Date.now < user.lockExpiry) {
        //some logic here
      } else if (!user.emailVerified) {
        //some logic here
      } else {

        bcrypt.compare(inpPassword, user.passwordHash).then((compareResult) => {

          // check if password matches
          if (compareResult) {

            //some logic here
            setLoginFailureCount(user, loginFailureCount).then(() => {

                setLastLogin(user)
                  .then(() => {
                    //some logic here
                  })
                  .catch((err) => {
                    //some logic here
                  });
              })
              .catch((err) => {
                //some logic here
              });

          } else {

            //login failed
            loginFailureCount = loginFailureCount + 1;
            setLoginFailureCount(user, loginFailureCount)
              .then(() => {
                //some logic here
              })
              .catch((err) => {
                //some logic here
              });
          }
        });
      }
    })
    .catch((err) => {
      //some logic here
    });
}

All looks good, and we feel that the promises are not used correctly; there are multiple catch statements.

One of my colleague said that:

Ideally, the code structure with promises shall look like this:

someFunction()
.then()
.then()
.then()
.catch();

Since, there are multiple if-else statements. We were trying to brainstorm if the above kind of chaining can be implemented or not.

Is there a better way to implement promises in this code?

Answer 1

This is missing the point. It is not about promises. You are right that if done correctly, you'll get a nice flat structure, but not if you inline all your code. This would be true if you weren't using promises as well. What you should do is split your code into smaller building blocks, and the nice structure will follow.

Also, be wary of your variable names:

// check if password matches
if (compareResult) {

Why not rename "compareResult" "passwordIsCorrect" and get rid of the comment?

// find the user
User.findOne({

That comment is useless.

Basically all the comments can be removed. Try to explain yourself with code instead, and only comment what can't be explained with code.

Just a quick example of the type of refactoring I would do:

routes.login = (req, res) => {
    let inpEmail = req.body.email.toLowerCase();
    let inpPassword = req.body.password;

    User.findOne({email: inpEmail})
        .exec()
        .then((user) => {
            if(user.isLocked && Date.now < user.lockExpiry){
                return blabla();
            }else if(!user.emailVerified){
                return requireEmailVerification();
            }else{
                return login(user, inpPassword);
            }
        })
        .then(() => {
            // Probably res.send() here
        })
        .catch((err) => {
            //some logic here
        });
};

function login(user, pw){
    return bcrypt.compare(pw, user.passwordHash)
        .then(passwordIsCorrect => {
            if(passwordIsCorrect){
                return setLastLoginAndDoSomeLogic(user);
            }

            return incrementLoginFailureCountAndDoSomeLogic(user);
        });
}

function setLastLoginAndDoSomeLogic(user){
    return setLastLogin(user)
        .then(() => {
            //some logic here
        })
        .catch((err) => {
            //some logic here
        });
}

function incrementLoginFailureCountAndDoSomeLogic(user){
    return setLoginFailureCount(user, user.loginFailureCount + 1)
        .then(() => {
            //some logic here
        })
        .catch((err) => {
            //some logic here
        });
}