Better code for error handling in NodeJs and Express

Question

I have this code for a post request, when the user wants to change his password. Because of all the cases and the following page renders the code came out really ugly. Is there a better way to structure this? (It works fine and does what I want.)

// Check if old password is correct
 SQL.getUserFromDB(request.session.username).then(function (results) {
 // Hash and compare with stored hash
 bcrypt.compare(request.body.oldPw, results[0].password, function (error, result) {
 // Log possible error
 if (error) console.log(error);
 if (result === true) {
 // Check if new passwords are both the same
 if (request.body.newPw === request.body.newPw2) {
 // Call mysql function
 SQL.changeUserPassword(request.session.username, request.body.newPw).then(function () {
 response.render('pages/changePassword', {
 user: request.session.username,
 text: 'Passwort erfolgreich geändert.'
 });
 }).catch(function (error) {
 console.log(error);
 if (error == 'pw') {
 response.render('pages/changePassword', {
 user: request.session.username,
 text: 'Neues Passwort zu unsicher.'
 });
 } else {
 // Render error page
 response.render('pages/changePassword', {
 user: request.session.username,
 text: 'Fehler beim Ändern des Passworts.'
 });
 }
 });
 } else {
 // Render error page
 response.render('pages/changePassword', {
 user: request.session.username,
 text: 'Neue Passwörter stimmen nicht überein!'
 });
 }
 } else {
 // Render error page
 response.render('pages/changePassword', {
 user: request.session.username,
 text: 'Altes Passwort stimmt nicht überein!'
 });
 }
 });
 // Catch sql errorsFehler beim Ändern des Passworts
 }).catch(function (error) {
 if (error) console.log(error);
 response.render('pages/errors/loginFailed');
 });

I tried just setting the text in the different cases and rendering one page with the text at the bottom, this didn't work however.

Answer 1

What you first need to do is convert all non-promise/callback-based operations into promises, or wrap them in one. This way, you can write seamless promise-based code. For instance, convert bcrypt.compare() into:

const bcryptCompare = (pw1, pw2) => {
 return new Promise((resolve, reject) => {
 bcrypt.compare(pw1, pw2, (error, result) => {
 if(error) reject(error)
 else resolve(result)
 })
 })
}

There are libraries that do this for you, and newer versions of libraries usually support promise-based versions of their APIs. But if you ever need to craft this yourself, this is how it's generally done.

Next, I would separate express from this logic. This way, you're not constrained with Express. Instead of accessing request, pass only the needed information to your function. Instead of immediately sending a response, consider throwing error objects instead. This way, you can defer your response logic to your routing logic.

Next, you could turn to async/await for cleaner code. It's just syntax sugar on top of Promises, allowing asynchronous code to look synchronous.

// Custom error class to house additional info. Also used for determining the
// correct response later.
class PasswordMismatch extends Error {
 constructor(message, user){
 this.message = 'Neue Passwörter stimmen nicht überein!'
 this.user = user
 }
}
class UserDoesNotExist extends Error {...}
class PasswordIncorrect extends Error {...}
// More custom error classes here
// Our logic, which knows nothing about express and only requires a few
// pieces of information for our operation.
const changePassword = async (username, oldPw, newPw, newPw2) => {
 // Throwing an error inside an async function rejects the promise it returns
 if (newPw !== newPw2) {
 throw new PasswordMismatch('Neue Passwörter stimmen nicht überein!', user)
 }
 const results = await SQL.getUserFromDB(username)
 if (!results) {
 throw new UserDoesNotExist('...', user)
 }
 const result = bcryptCompare(oldPw, results[0].password)
 if(result !== true) {
 throw new PasswordIncorrect('...', user)
 }
 try {
 SQL.changeUserPassword(username, newPw)
 } catch(e) {
 throw new PasswordChangeFailed('...', user)
 }
}
// In your express router
app.post('route', async (request, response) => {
 // Gather the necessary data from the request
 const username = request.session.username
 const oldPw = request.body.oldPw
 const newPw = request.body.newPw
 const newPw2 = request.body.newPw2
 try {
 await changePassword(username, oldPw, newPw, newPw2)
 // If the operation didn't throw, then we're good. Happy path.
 response.render('pages/changePassword', {
 user: username,
 text: 'Passwort erfolgreich geändert.'
 });
 } catch (e) {
 // If any of the operations failed, then we determine the right response
 // In this case, we used a custom error. We can get the data from it.
 // You can have more of these for the other custom classes.
 if(error instanceof PasswordConfirmError){
 response.render('pages/changePassword', {
 user: error.user,
 text: error.message
 });
 } else {
 // Generic 500 error, if all else fails.
 }
 }
})

• user3742929: Thanks for the in-depth answer. That looks way nicer than what i hacked together.
• user3742929: Would you normally seperate the logic in different files?
• Joseph: @user3742929 Depends on your case. Small script, you don't worry about it. But when the code grows, separate them. Also, one thing to consider is that changePassword in my example could be used regardless of the presence of express.