Anti-Pattern to aggregate errors and pass to promise resolve js?

Question

I'm processing a list in a loop that's run async returning a promise and I do not want to exit processing on exception, so I aggregate them and pass them to the resolve callback in an outer finally block. I'll like to know if this is an anti-pattern and if so, please provide a pointer on how to do correctly. Thanks.

Example

async doSomething(list) {
let errorCount = 0
let errors = []
return new Promise(async (resolve, reject) => {
  try {
    list.forEach(async (item) => {
      try {
        actionThatThrows(item)
      } catch (e) {
        errorCount++
        errors[errorCount] = e
      }
    })
  } catch (e) {
    errorCount++
    errors[errorCount] = e
  } finally {
    if (errorCount > 0) {
      resolve(errors)
    } else {
      resolve()
    }
  }
})

}

This isn't 100% clear, especially what's the "outer" finally block. Can you please add an example in your question? — Denys Séguret, Dec 21 '16 at 07:31
Why is there an outer try/catch ? It doesn't look like it could catch anything — Denys Séguret, Dec 21 '16 at 07:39
The errors are the result of your asynchronous computation. This could be written in a simpler way (and even clearer with Bluebird's map) but this isn't a known antipattern. — Denys Séguret, Dec 21 '16 at 07:41
Is `actionThatThrows` asynchronous ? It doesn't look like it is in your code but then there's nothing asynchronous. — Denys Séguret, Dec 21 '16 at 07:55

score 8 · Accepted Answer · edited May 23 '17 at 10:30

Yes, this code employs several antipatterns:

I do not want to exit processing on exception but aggregate them

You might want to have a look at Wait until all ES6 promises complete, even rejected promises for that.

But you can do it without those as well, assuming you want sequential iteration:

async function doSomething(list) {
    const errors = [];
    for (let item of list) {
        try {
            await actionThatThrows(item);
        } catch (e) {
            errors.push(e);
        }
    }
    if (errors.length)
        return errors;
    else
        return …;
}

Denys Séguret · Answer 2 · 2016-12-21T08:12:25.313

The errors are the result of your asynchronous computation so it globally looks legit.

Assuming that actionThatThrows returns a promise (it's unclear in your question and code), it looks like it could be written like this:

function doSomething(list) {
    let errors = []
    return Promise.all(list.map(
        item => actionThatThrows(item).catch(e => {
            errors.push(e);
        })
    )).then(()=>{
        return errors.length ? errors : undefined;
    });
}

score 0 · Answer 3 · answered Dec 21 '16 at 08:00

1) async doSomething is not invoking an await, so remove async 2) async in list.forEach is not invoking an await, so remove async 3) First catch will catch all. Second catch will never be hit, so remove second catch and finally

Code can be simplified to:

doSomething(list) {
    let errorCount = 0,
        errors = [];
    for (let item of list) {
        try {
            actionThatThrows(item); //I suppose this is not returning a promise
        } catch (e) {
            errorCount += 1;
            errors[errorCount] = e;
        }
    }
    if (errorCount > 0) {
        return errors; //return Promise.reject(errors);
    } else {
        //return Promise.resolve();
    }
}

Anti-Pattern to aggregate errors and pass to promise resolve js?

3 Answers3