Is it a mistake to pass a Promise resolve/reject handler to a subfunction?

Question

Considering that I have code that uses callbacks and I'm trying to wrap it in promises, is the following code a mistake or anti-pattern?

function getDateAsync(onSuccess, onFailure) {
   ...
}

function getValidDate() {
    return new Promise((resolve, reject) => {
       getDateAsync((date) => {
         checkValidDate(date, resolve, reject);
       }, () => {
         markDateInvalid();
         reject();
       }
    });
}

function checkValidDate(date, resolve, reject) {
   if (isValid(date)) {
      resolve(date);
   } else {
      markDateInvalid();
      reject();
   }
}

function markDateInvalid() { ... }

This is assuming that checkValidDate is more complicated, and can't be inlined, and that markDateInvalid needs to be called in both of the instances indicated.

Is there a better way to write this code?

Update: For the sake of this example, we should also assume that getDateAsync is an external library, or is otherwise expensive and infeasible to convert to using Promises.

I guess it depends where the actual *asynchrony* is - in your code there doens't seem to be any reason to use Promises in the first place — Bravo, Nov 14 '18 at 20:36
Pretend that the rest of the code base uses Promises, and getDateAsync is making a remote call and using callbacks. So we want getValidDate to use promises as well. — audiodude, Nov 14 '18 at 20:38
`getDateAsync()` should be returning promise then you don't have to create new promise in `getValidDate()` — charlietfl, Nov 14 '18 at 20:38
@charlietfl True, but I meant to imply that getDateAsync is provided by a library or external code and can't be made to use promises. I'll update. — audiodude, Nov 14 '18 at 20:40
Might want to look at libraries and other solutions that *"promisify"* callbacks. https://stackoverflow.com/questions/22519784/how-do-i-convert-an-existing-callback-api-to-promises — charlietfl, Nov 14 '18 at 20:47
`Pretend that the rest of the code base uses Promises` ... `getDateAsync is provided by a library or external code and can't be made to use promises` ... so ... which is it? — Bravo, Nov 14 '18 at 22:07

score 2 · Answer 1 · answered Nov 14 '18 at 20:40

Over all the concept seems fine, the only issue I see is that it would be difficult for a person to understand when looking at it. Might I suggest a minor change to your structure?

function getDateAsync(onSuccess, onFailure) {
   ...
}

function getValidDate() {
    return new Promise((resolve, reject) => {
       getDateAsync((date) => {
         if (isValidDate(date)) {
            resolve(date);
         } else {
            markDateInvalid();
            reject();
         }
       }, () => {
         markDateInvalid();
         reject();
       }
    });
}

function checkValidDate(date) {
   // return a boolean 
}

function markDateInvalid() { ... }

It makes more sense to keep the resolve & reject inside of the initial creation of the promise and make checkValidDate return a boolean.

Key Take Away:

If you have to develop this weird pattern to begin with, then it's a good sign that your checkValidDate function is too complicated and needs to be reworked. Your code doesn't defeat the purpose of using promises, but it does seem to be a bit overly complicated.

score 2 · Accepted Answer · answered Nov 14 '18 at 20:50

Yes, I would consider this an antipattern. If you need to promisify an external callback-taking function, you should wrap only that function and nothing else. Write a wrapper

function getDateAsPromise() {
    return new Promise(getDateAsync);
    // return new Promise((resolve, reject) => { getDateAsync(resolve, reject); });
}

and from then on use only promises:

function getValidDate() {
    return getDataAsPromise().then(date => {
        return checkValidDate(date);
    }, () => {
        markDateInvalid();
        throw; // … some useful value
    });
}

function checkValidDate(date) {
    if (isValid(date)) {
        return date;
    } else {
        markDateInvalid();
        throw; // … some useful value
    }
}

When there's an exception thrown in the callbacks (or checkValidDate), this will still work. With your original approach - and standard callback-based code - the whole process would fail with an error.

Is it a mistake to pass a Promise resolve/reject handler to a subfunction?

2 Answers2