Am I using promises correctly?

Question

I have the following function (I'm using the Q promise library):

confirmEmail: function(confirmationCode){
    var deferred = q.defer();

    User.find({
      where: {confirmation_code: confirmationCode}
    }).then(function(user){
      if(user){
        user.updateAttributes({
          confirmation_code : null,
          confirmed: true
        }).then(function() {
          deferred.resolve();
        });
      }else{
        deferred.reject(new Error('Invalid confirmation code'));
      }
    });

    return deferred.promise;
  }

I've been reading a bit about the best practices regarding promises e.g. What is the explicit promise construction antipattern and how do I avoid it? http://pouchdb.com/2015/05/18/we-have-a-problem-with-promises.html

Have I written the above function so that it is keeping with these practices, or is there a better way?

Just wondering - what was unclear in the example here? http://stackoverflow.com/questions/23803743/what-is-the-deferred-antipattern-and-how-do-i-avoid-it — Benjamin Gruenbaum, May 19 '15 at 14:10

robertklep · Accepted Answer · 2015-05-19T14:20:33.987

3

It seems to me like you can rewrite your method to this:

confirmEmail : function(confirmationCode) {
  return User.find({
    where : { confirmation_code : confirmationCode }
  }).then(function(user) {
    if (! user) {
      throw new Error('Invalid confirmation code');
    }
    return user.updateAttributes({
      confirmation_code : null,
      confirmed         : true
    });
  });
}

Both User.find() and user.updateAttributes() seem to be returning promises (I'm inferring this from your code), so you can easily create a promise chain with them.

But even if they weren't returning promises, you probably wouldn't have needed q.defer(), as outlined on this page you already mention ("Rookie mistake #4"). See Q.Promise.

edited May 19 '15 at 14:20

answered May 19 '15 at 14:05

robertklep

198,204
35
394
381

I'm not sure about the control flow. The confirmEmail method is returning the User.find promise, so if you call a `then()` on it, you'll get the result of that promise, and not on the `user.updateAttributes` one – ItalyPaleAle May 21 '15 at 15:08
@Qualcuno: No, `confirmMail` returns the *result of calling `.then()`* on the `User.find()` promise, so it's a promise that resolves with the result of `user.updateAttribute()`. – Bergi May 21 '15 at 16:58
@Bergi I understand! what about the fact that rejections aren't handled in user.updateAttributes? – ItalyPaleAle May 21 '15 at 16:59
@Qualcuno: Right, rejections of `updateAttributes()` would be promoted here. To match the original code, one would have to add `.catch(ignoreError)` (although I don't hink that's a good idea). – Bergi May 21 '15 at 17:01
@Qualcuno error handling is left to the caller of `confirmMail()`. I specifically didn't ignore any errors on `user.updateAttributes()`—which is what the original code was doing—because it seemed like an oversight and not intentional. – robertklep May 21 '15 at 19:47

score 0 · Answer 2 · answered May 21 '15 at 15:19

With Mongoose (especially from version 4), promises are supported natively, so you don't need to use Q. Also, on Node.js 0.12+ and io.js there's natie support for Promises, so again no need for Q!

This is how I would write the method (using the native Promise; here a polyfill if still on Node 0.10)

confirmEmail: function(confirmationCode){
    return new Promise(function(resolve, reject) {
        User.find({
            where: {confirmation_code: confirmationCode}
        })
        .then(function(user) {
            if(user){
                user.updateAttributes({
                    confirmation_code : null,
                    confirmed: true
                })
                .then(resolve, reject)
            }
            else {
                reject(new Error('Invalid confirmation code'))
            }
        }, reject)
    })
}

How it works: 1. The confirmEmail method returns one Promise. As per pattern, the promise can be either resolve()'d or reject()'d 2. User.find is invoked. With Mongoose, that returns a promise, so you can do: then(callback, reject). So, if User.find returns an error, the Promise returned by confirmEmail will be rejected (the "outer one"). 3. If User.find succeeds, we proceed. If there is no result (if(user) is false), then we manually reject the "outer promise" and we don't do anything else. 4. If there's a user, instead, we call user.updateAttributes, which too returns a promise. We're thus invoking then(resolve, reject) so the result of that promise will be passed to the "outer promise".

Example use:

obj.confirmEmail(confirmationCode).
.then(function(user) {
    // Everything went fine
}, function(error) {
    // This is invoked if: User.find() failed, or if no result was obtained (with the error "Invalid confirmation code") or if user.updateAttributes failed
})

That's just the [very same antipattern](http://stackoverflow.com/q/23803743/1048572) as the OP uses. — Bergi, May 21 '15 at 17:27

Am I using promises correctly?

2 Answers2