1
\$\begingroup\$

I have a route I've rewrote to use promises.

router.post(
  '/forgot-password',
  (req, res) => {
    //generate random password
    let passwordString = Math.random().toString(36).slice(-10);

    User
      .findOne({'username': req.body.email})
      .then(foundUser => {
        foundUser.setPassword(
          passwordString,
          () => {
            foundUser.save(
              () => {
                emailService.sendForgotPassword(foundUser, passwordString);
                req.flash('success', 'Please check your email.');
                res.redirect('/login');
              });
          });
      })
      .catch(err => {
        if(err && err.message) req.flash('error', 'Cannot find a user by that email.');
        res.redirect('back');
      });
  });

I think my then route could be better than what it is now. How do I optimize the promise chain a little better?

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

You can't optimize a promise chain that doesn't exist. The only method that's returning a promise here is User.findOne(). In order for the route to be promise based, you'll need to rewrite the other methods you are calling inside the route.

I'll do my best, with the small amount of information you provided, to help you figure out how to change it.

foundUser.setPassword() seems to be asynchronous but it's using the callback pattern: foundUser.setPassword( newPassword, functionToCallWhenDone ). You need to rewrite it so instead of accepting a function to call back once done, it should return a promise as soon as it finishes or errors out. That way it can be chained with the other promises in the route.

The same can be said about foundUser.save( functionToCallWhenDone ).

Also emailService.sendForgotPassword( foundUser, passwordString ) seems to be crucial to the request's success. I recommend you to rewrite it so it returns a promise too, and catch the error outside the main chain to handle it.

After applying these changes your code could look like this:

// Instead of using comments to indicate your intention, abstract the logic into a method
function generateRandomPassword() {
  return Math.random().toString( 36 ).slice( - 10 );
}

router.post(
  '/forgot-password',
  ( req, res ) => {
    let newPassword;
    let user;

    // I like to start with Promise.resolve() as that makes all the other method calls be at the same indentation level
    // Note: This is not required and purely just about style
    // Also, wrapping anything you'll do around a promise's then method protects you from unforeseen errors
    Promise.resolve()
      // TODO: What happens if the email wasn't sent with the request?
      .then( () => User.findOne( { 'username': req.body.email } ) )
      .then( _user => {
        user = _user;

        newPassword = generateRandomPassword();
        return user.setPassword( newPassword );
      } )
      .then( () => user.save() )
      .then( () => emailService.sendForgotPassword( user, newPassword )
        .catch( error => {
          // What happens if the emailService fails? 
          // Here you might want to revert changing the user's password
          revertPasswordChange();

          // Don't forget to return a rejected promise (or throw again the error) because if not, the next then will wrap
          // whatever you return here (or an empty value if you didn't return something) and the success message will be
          // wrongly displayed.
          return Promise.reject( error );
        } )
      )
      .then( () => {
        req.flash( 'success', 'Please check your email.' );
        res.redirect( '/login' );
      } )
      .catch( error => {
        if( error && error.message ) req.flash( 'error', error.message );
        res.redirect( 'back' );
      } )
    ;
  }
);

Note that this code relies on mutable contextual state, which has some drawbacks and (depending on the context) may be considered an anti-pattern. Here's a very good read about how to share state between promises.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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