3

According to this link (Rookie mistake #2) I should not use loops within Promises, but instead Promise.all(iterable).

Does this really apply to all loops? Promise.all(iterable) takes an array of size n. If I'd use Promise.all(iterable), then I'd get as a result (i.e. iterable_A) an array of the size n.

What if I want to iterate through iterable and only want to put certain elements that satisfy my condition to another iterable (e.g. iterable_B) and want to return the iterable_B instead of iterable_A? Should I use Promise.all() too?

thadeuszlay
  • 2,787
  • 7
  • 32
  • 67
  • 2
    You can use loops in promises as long as they don't take much time to execute and you don't perform any asynchronous tasks in the loop. – Tesseract Jun 16 '15 at 15:41
  • Ah, yeah. That makes sense. The article in the link above wasn't clear about that. It made the impression all loops are forbidden with promises. – thadeuszlay Jun 16 '15 at 15:44
  • possible duplicate of [Correct way to write loops for promise.](http://stackoverflow.com/questions/24660096/correct-way-to-write-loops-for-promise) – djechlin Jun 16 '15 at 15:57

1 Answers1

4

I should not use loops within Promises

No, rather the other way round: You should not use promises within loops.

Of course that's too generic as well. Sometimes you just need a loop structure. What you must not do is forget to collect the promises that are created in the loop body in some iterable that you can pass to Promise.all, to await all the asynchronous things started in that loop.

The map method as suggested in the article naturally does that, you just have to return a promise from the callback (as always). Using for/while/.forEach makes it a bit harder as you have to manually push the promises in some array (which is not only ugly but also errorprone).

However, if you are not dealing with asynchronous tasks inside your loop, you can do whatever you want. For example, both

Promise.all(values.filter(syncPredicate).map(asyncFn))

and

Promise.all(promises).then((values) => values.filter(syncPredicate))

are totally fine. It does become a bit more complicated when you have an asynchronous filter predicate, I'd recommend to look out for a promise utility library in that case.

Also you will have to realise that asynchronous tasks started from within a synchronous loop construct will run in parallel. If you intend to run them sequentially (await each iteration), you should try to formulate the loop using a recursive structure.

Community
  • 1
  • 1
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • In your opinion should the following function be refactored? I'm specifically referring to the callback, defer, and the for-loop in that function: – thadeuszlay Jun 30 '15 at 13:36
  • `var getAllFooById = function(id){ var dfd = $q.defer(); db.allDocs({include_docs: true}, function(err, response) { if(err) { console.log(err); dfd.reject(err); } else { var fooArray = []; for(var i=0; i < response.total_rows; i++) { if(response.rows[i].doc.type == 'foo' && response.rows[i].doc.id == id) { fooArray.push({bar: response.rows[i].doc._id}); } } dfd.resolve(fooArray); } }); return dfd.promise; };` – thadeuszlay Jun 30 '15 at 13:38
  • Is that a new question? An [answer](http://stackoverflow.com/help/self-answer)? I cannot really decipher unformatted code in comments, but it looks like you are using the [deferred antipattern](http://stackoverflow.com/q/23803743/1048572) – Bergi Jun 30 '15 at 13:38
  • Sorry for the formatting. It's a question to what you said: "if you are not dealing with asynchronous tasks inside your loop, you can do whatever you want.": Is `response` asynchronous or not? Should I leave the `for`-loop as it is right now or change it into `Promise.all()`? – thadeuszlay Jun 30 '15 at 13:45
  • @thadeuszlay: I don't know what `db.allDocs` is so I can't say for sure, but it looks certainly asynchronous. If it wasn't, you wouldn't have to use deferreds. But yes, the `for` loop in that callback doesn't do anything asynchronous, it's just a synchronous loop over `response`, no need to use `Promise.all` inside there. However, your callback should only do `if (err) dfd.reject(err); else dfd.resolve(response);` and nothing else, all that `console.log` and looping things should go in a separate `.then()` callback. – Bergi Jun 30 '15 at 13:50
  • I think you have to replace it. But I'm not sure. My reasoning: I'm accessing data from pouchDB. Response is therefore an asynchronous construct. Thus I have to use Promise.all(). Am I right? EDIT: Ah, thanks. So in the for loop I got all my responses and therefore don't need Promise.all()? – thadeuszlay Jun 30 '15 at 13:53
  • If the `db.allDocs` callback fires only once, and you don't call `db.allDocs` multiple times, then yes you don't need `Promise.all` - `response.rows` seems to be an array with all results. Otherwise, you run into trouble. – Bergi Jun 30 '15 at 13:58
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/81978/discussion-between-thadeuszlay-and-bergi). – thadeuszlay Jun 30 '15 at 14:04