2
\$\begingroup\$

I am in the process of refactoring some complex code that uses callbacks to instead use promises. I am trying to figure out if my proposed approaches make sense.

/*
  Current implementation.
  Hard to test because of the intermingling of various controller functionality,
  such as preparing responses.
  Gets a little deep on the nesting.
 */

controller.authenticate = function(req, res) {

  var customerCode = req.body.customerCode;
  var accountType = req.body.accountType;
  var username = req.body.username;
  var password = req.body.password;
  if (!(customerCode && accountType && username && password)) return helper.handleInvalidRequest(res);

  Account.findByAccountTypeAndUsername(accountType, username, function(err, account) {

    if (err) return helper.handleError(res, err);
    if (!account) return helper.handleNotFound(res);

    account.authenticatePassword(password, function(err, authenticated) {

      if (err) return helper.handleError(res, err);

      if (account.status == 'Locked') {
        var pass = new VisitorPass({accountID: account._id});
        return pass.save(function(err) {
          if (err) return helper.handleError(res, err);
          messaging.sendAccountLockedMessage(customerCode, account, pass, null);
          return helper.handleLockedAccount(res, pass);
        });
      }

      if (!authenticated) return helper.handleNotAuthenticated(res);

      if (account.tfaEnabled && !account.deviceIsTrusted(req.trustedDeviceToken)) {
        if (account.tfaMethod == 'sms') {
          var hotp = account.getNextHotp();
          return account.save(function(err) {
            if (err) return helper.handleError(res, err);
            messaging.sendOneTimePasswordAuthenticationMessage(account.tfaPhone, hotp, null);
            return helper.handleTfaRequired(res, account, auth.createPartiallyAuthenticatedJWT(account._id));
          });
        }
        else {
          return helper.handleTfaRequired(res, account, auth.createPartiallyAuthenticatedJWT(account._id));
        }
      }
      else {
        res.status(201).send({token: auth.createFullyAuthenticatedJWT(account._id)});
      }

    });

  });

};

First approach at refactoring:

/*
  Moves work to a new AccountService.authenticate method, which would return
  a promise.
 */

controller.authenticate = function(req, res) {

  var customerCode = req.body.customerCode;
  var accountType = req.body.accountType;
  var username = req.body.username;
  var password = req.body.password;

  AccountService.authenticate(customerCode, accountType, username, password)
    .then(function(token) {
      res.status(201).send({token: token});
    })
    .catch(InvalidRequestError, function() {
      helper.handleInvalidRequest(res);
    })
    .catch(NotFoundError, function() {
      helper.handleNotFound(res);
    })
    .catch(AccountLockedError, function(err) {
      return helper.handleLockedAccount(res, err.pass);
    })
    .catch(NotAuthenticatedError, function() {
      helper.handleNotAuthenticated(res);
    })
    .catch(TwoFactorAuthRequiredError, function(err) {
      helper.handleTfaRequired(res, err.account, err.token);
    })
    .catch(function(err) {
      helper.handleError(res, err);
    });

};

Second approach to refactoring:

/*
 In-between alternative. Still handles request validation itself.
 Modifies two methods (Account.findByAccountTypeAndUsername and account.authenticate)
 to return promises.
 */

controller.authenticate = function(req, res) {

  var customerCode = req.body.customerCode;
  var accountType = req.body.accountType;
  var username = req.body.username;
  var password = req.body.password;
  if (!(customerCode && accountType && username && password)) return helper.handleInvalidRequest(res);

  Account.findByAccountTypeAndUsername(accountType, username)
    .then(function(account) {
      return account.authenticate(password)
        .then(function(token) {
          res.status(201).send({token: token});
        })
        .catch(AccountLockedError, function(err) {
          return helper.handleLockedAccount(res, err.pass);
        })
        .catch(NotAuthenticatedError, function() {
          helper.handleNotAuthenticated(res);
        })
        .catch(TwoFactorAuthRequiredError, function(err) {
          helper.handleTfaRequired(res, err.account, err.token);
        });
    })
    .catch(NotFoundError, function() {
      helper.handleNotFound(res);
    })
    .catch(function(err) {
      helper.handleError(res, err);
    });

};

I have a few specific questions:

  1. Are these good scenarios for using promises over callbacks?
  2. Is the use of custom error types like AccountLockedError acceptable in this kind of scenario?
  3. In the third example, because the first promise was fulfilled with the "account" object, I had to nest the next call and promise chain. Is there a better approach to this?
\$\endgroup\$

migrated from programmers.stackexchange.com Feb 20 '15 at 3:24

This question came from our site for professionals, academics, and students working within the systems development life cycle.

  • \$\begingroup\$ Be careful about saying "refactoring" when you really just mean "rewriting". \$\endgroup\$ – 200_success Feb 20 '15 at 7:04

Your Answer

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

Browse other questions tagged or ask your own question.