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:
- Are these good scenarios for using promises over callbacks?
- Is the use of custom error types like
AccountLockedErroracceptable in this kind of scenario? - 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?
