0

I have the following method that I'm trying to complete:

  getAllValues: function (callback) {
    this.getCount((count) => { // count is usually 5
      let results = []
      for (var i = 0; i < count; i++) {
        this.getValue(i, (result) => { // getValue() is async and eventually returns a string
          results.push(result)
        })
        if (i == count-1) {
          callback(results)
        }
      }

I want results to be an array with all of the strings returned by getValue(); however, I haven't been able to figure out how to do this. In callback(results), results ends up being an empty array, so the pushed values are being dropped somehow

How do I get this to do what I want?

EDIT: I do NOT want to use promises here.

D Day
  • 1
  • 2
  • Are you able to use promises instead? There’s a very convenient `Promise.all` that exists. Doing the same thing otherwise involves keeping track of how many callbacks have been called so far. – Ry- Nov 30 '17 at 23:22
  • I think this can help you: https://gist.github.com/anvk/5602ec398e4fdc521e2bf9940fd90f84 – JJJ Nov 30 '17 at 23:25
  • If getValue is async then you need to use await to get the result, not just get the promise it returns – John Nov 30 '17 at 23:31
  • @John - `await` only works if `getValue` returns a promise – Jaromanda X Nov 30 '17 at 23:32
  • @JosanIracheta - you too assume that getValue returns a Promise, there's no indication that this is the case, since it uses the "callback" pattern, it's highly unlikely to return a Promsie – Jaromanda X Nov 30 '17 at 23:43
  • @JaromandaX I'm not assuming that it returns a promise but I was hoping that maybe he would consider re-writing his code to use promises – JJJ Nov 30 '17 at 23:46
  • 1
    fair enough @JosanIracheta – Jaromanda X Nov 30 '17 at 23:46
  • this stackoverflow answer should help and uses no promises: https://stackoverflow.com/questions/13343340/calling-an-asynchronous-function-within-a-for-loop-in-javascript – JJJ Nov 30 '17 at 23:47
  • how so @JosanIracheta? there's no logic in that answer regarding when to call the call back ... I think [this answer](https://stackoverflow.com/a/47584358/5053002) is more relevant :p it too uses no Promises, at least, not the first snippet – Jaromanda X Nov 30 '17 at 23:49
  • @JaromandaX that SO question is about async functions within a for-loop. It's very relevant. But yes, your answer is good too lol ;-) – JJJ Nov 30 '17 at 23:54
  • It is relevant @JosanIracheta, but the problem the OP has is calling the callback at the wrong time – Jaromanda X Nov 30 '17 at 23:55
  • Sorry @Jaromanda X, I read the comment in the code and thought that it was referring to an actual async function and not simply something that runs asynchronously in the more traditional node callback style. – John Nov 30 '17 at 23:57
  • ahh yes, @John - I can see how that happened :p – Jaromanda X Nov 30 '17 at 23:58

2 Answers2

2

You're testing for the results completion in the wrong place

getAllValues: function(callback) {
    this.getCount((count) => { // count is usually 5
        let results = [];
        let completed = 0;
        for (let i = 0; i < count; i++) { // *** use let instead
            this.getValue(i, (result) => { // getValue() is async and eventually returns a string
                completed ++;
                results[i] = result; // *** note this change to guarantee the order of results is preserved
                if (completed == count) {
                    callback(results)
                }
            })
        }
    })
}

Note: use let in the for loop, so that i is correct inside

don't push ... assign to the index, to preserve order of results

and alternative, by having a "promisified" getValue (called it getValuePromise in the code below)

getValuePromise: function(i) {
    return new Promise(resolve => {
        this.getValue(i, resolve);
    });
}
getAllValues: function(callback) {
    this.getCount((count) => 
        Promise.all(Array.from({length:count}).map((unused, i) => this.getValuePromise(i)))
        .then(callback)
    );
}
Jaromanda X
  • 53,868
  • 5
  • 73
  • 87
-2

What you need to do is use then() it will wait for your async function to finish then it will run your callback.

getAllValues: function (callback) {
    this.getCount((count) => { // count is usually 5
      let results = []
      for (var i = 0; i < count; i++) {
        this.getValue(i, (result) => { // getValue() is async and eventually returns a string
          results.push(result)
        }).then(function(getValueResults){
            if (i == count-1) {
              callback(results)
            }
        })
      }
Get Off My Lawn
  • 34,175
  • 38
  • 176
  • 338
  • I think the problem is that the loop is over before even one result is returned from the async function – JJJ Nov 30 '17 at 23:25
  • 1
    `What you need to do is use then()` assuming `getValue` returns a Promise as well as calling a callback - an unlikely situation, most asynchronous functions do one or the other, not both – Jaromanda X Nov 30 '17 at 23:28
  • His comment says it is async, so I assumed that it returned a promise... – Get Off My Lawn Nov 30 '17 at 23:29
  • 1
    async code existed since Brendan Eich first created javascript - Promises have not :p – Jaromanda X Nov 30 '17 at 23:32
  • `most asynchronous functions do one or the other, not both` - to clarify, there's no reason why it can't do both, other than lack of clarity in code :p – Jaromanda X Nov 30 '17 at 23:47