How to properly use resolve and reject for promises

Question

I've started to look at using Promises and have begun by putting together a simple function and calling it a few times. I need a sanity check around reject and resolve.

1. Is this the correct way to "promisify" a function?
2. Is this the correct way to deal with reject and resolve?

3. Anything I've got totally wrong?

   const Redis     = require('ioredis');
   const redis     = new Redis({
       port: 6379,
       host: '127.0.0.1'
   });
   
   
   function checkValues(name, section) {
       return new Promise((resolve, reject) => {
           redis.multi()
           .sismember('names', name)
           .sismember('sections', section)
           .exec()
           .then((results) => {
               if(results[0][1] === 1 && results [1][1] ===1) {
                   reject('Match on both.');
               } else if(results[0][1] === 1 || results [1][1] ===1) {
                   reject('Match on one.');
               } else {
                   redis.multi()
                   .sadd('names', name)
                   .sadd('sections', section)
                   .exec()
                   .then((results) => {
                       // Lazy assumption of success.
                       resolve('Added as no matches.');
                   })
                   // No catch needed as this would be thrown up and caught?
               }
           })
           .catch((error) => {
               console.log(error);
           });
       });
   }
   
   
   // Call stuff.
   checkValues('barry', 'green')
   .then((result) => {
       // Added as no matches "resolve" message from 'barry', 'green'
       console.log(result); 
       retutn checkValues('steve', 'blue');
   })
   .then((result) => {
       // Added as no matches "resolve" message from 'steve', 'blue'
       retutn checkValues('steve', 'blue');
   })
   .then((result) => {
       // Match on both "reject" message from 'steve', 'blue'
       console.log(result);
   })
   .catch((error) => {
       console.log(error);
   });
   

Answer 1

No, this is kind of an anti-pattern. You already have a function that returns a promise so you don't need to wrap it in another promise, you can just return it. Remember that then() returns a promise that resolves to the return value of then. You can also return another promise from then. Usually this looks super clean, but in this case you need some logic in the then function, so it gets a little messy.

function checkValues(name, section) {
  // Just return this Promise
  return redis.multi()
    .sismember('names', name)
    .sismember('sections', section)
    .exec()
    .then((results) => {
        if(results[0][1] === 1 && results [1][1] ===1) {
            // Rejections will be caught down the line
            return Promise.reject('Match on both.');
        } else if(results[0][1] === 1 || results [1][1] ===1) {
            return Promise.reject('Match on one.');
        } else {
            // You can return another Promise from then()
            return redis.multi()
            .sadd('names', name)
            .sadd('sections', section)
            .exec()
        }
    })
 // You don't need to catch here - you can catch everything at the end of the chain
}

Answer 2

Several points:

1. Don't use the explicit-promise-construction-antipattern
2. As a general guide in purging the anti-pattern, after removing the new Promise() wrapper, change resolve statements to return and reject statements to throw new Error(...).
3. .catch() catches! If errors are to be observable/handleable by the caller, then either don't catch in checkValues() or catch and re-throw. Catching without re-throwing will cause the returned promise to settle on its success path, never its error path, which is great for error recovery but not always appropriate.
4. Suggest that all three cases, 'Match on both', 'Match on one' and 'Added as no matches', are really successes. Unless there's a particular reason for wanting 'Match on both' and 'Match on one' to be seen as error conditions, then return rather than reject/throw. That way, your call stuff chain will progress down its success path, .then().then().then(), regardless of expected outcome; only an unexpected error will go down the error path to be caught by the final .catch(). This isn't a general rule; very often, throwing is the right thing to do, but not here.

function checkValues(name, section) {
    return redis.multi()
    .sismember('names', name)
    .sismember('sections', section)
    .exec()
    .then((results) => {
        if(results[0][1] === 1 && results [1][1] === 1) {
            return 'Match on both.';
        } else if(results[0][1] === 1 || results [1][1] ===1) {
            return 'Match on one.';
        } else {
            return redis.multi()
            .sadd('names', name)
            .sadd('sections', section)
            .exec()
            .then((results) => {
                return 'Added as no matches.';
            });
        }
    })
    .catch((error) => {
        console.log(error);
        throw error;
    });
}

// Call stuff.
checkValues('barry', 'green')
.then((result) => {
    console.log(result); // expect 'Added as no matches'
    return checkValues('steve', 'blue');
})
.then((result) => {
    return checkValues('steve', 'blue'); // expect 'Added as no matches'
})
.then((result) => {
    console.log(result); // expect 'Match on both'
})
.catch((error) => {
    // only an unexpected error will land here
    console.log(error);
});