4
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.
    }
  }
})
\$\endgroup\$
  • \$\begingroup\$ Thanks for the in-depth answer. That looks way nicer than what i hacked together. \$\endgroup\$ – user3742929 Jul 3 at 12:55
  • \$\begingroup\$ Would you normally seperate the logic in different files? \$\endgroup\$ – user3742929 Jul 3 at 13:00
  • \$\begingroup\$ @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. \$\endgroup\$ – Joseph Jul 3 at 13:51

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.