nodejs and mongodb (mongojs): Trying to query and update database within a for loop

Question

I'm writing a multiplayer game(mongojs, nodejs) and trying to figure out how to update user stats based on the outcome of the game. I already have the code written to compute all the post game stats. The problem comes when I try to update the users' stats in a for loop. Here's what i got:

//Game Stats
var tempgame = {
    gameid: 1234,
    stats: [
        {
            score: 25,
            user: 'user1'
        },
        {
            score: 25,
            user: 'user2'
        }
    ]
}


for(i = 0; i < tempgame.stats.length; i++){
    db.users.find({ username: tempgame.stats[i].user }, function(err, res){
        if( err != null){
            //handle errors here.
        } else {
            var userstats = res[0].stats;
            if( tempgame.stats[i].score > userstats.bestscore ){ //this is where it chokes 
                userstats.bestscore = tempgame.stats[i].score;
            }

            //code here to pass back new manipulated stats
        }
    });
}

Everything works fine until i try to use the tempgame object within the callback function. It says "cannot read property 'score' of undefined". Is this just a scoping issue?

Also i was thinking it could be an issue with the callback function itself. Maybe the loop would increment before the callback is even run. But even in that case, the score should be be there it would just be pulling from the wrong array index... that's what lead me to believe it may just be a scope issue.

Any help would be greatly appreciated.

score 7 · Accepted Answer · edited May 23 '17 at 11:45

7

You've been tripped up by the notorious "defining functions inside a loop" problem.

Use "forEach" instead:

tempgame.stats.forEach(function (stat) {
    db.users.find({ username: stat.user }, function(err, res){
        if( err != null){
            //handle errors here.
        } else {
            var userstats = res[0].stats;
            if( stat.score > userstats.bestscore ){ //this is where it chokes 
                userstats.bestscore = stat.score;
            }

            //code here to pass back new manipulated stats
        }
    });
});

edited May 23 '17 at 11:45

Community

1
1

answered Jan 22 '13 at 19:13

mjhm

16,497
10
44
55

This is just wrong, defining functions in a loop would not cause the behavior stated in the question. Additionally, you are still defining the functions within a loop, you are just doing it within a different function. `forEach` is still a loop. – Russ Bradberry Jan 22 '13 at 19:30
5

Actually it would cause this behavior. The issue is that the anonymous function defines a new closure, but it closes over the variable i, not the value of i at the time. So at the time the completion function runs, the value of i will equal tempgame.stats.length, and will be so for every execution of the callback function. One way to address this would be to pass the value of i in through another anonymous function - (function (i) { return function (err, res) { ... }; )(i) - or using foreach as shown above. – Chris Tavares Jan 22 '13 at 19:35
I see what you're saying, so I retract my previous statement. I however still believe that this does not fully solve the problem and the end result will be similar as there is no flow control over the database calls. This might work inside the cli, but it will fail on the next line of executable code, after the loop. – Russ Bradberry Jan 22 '13 at 20:45
this seemed to work just fine for the task at hand. Thanks guys! – v3xx3d Jan 24 '13 at 02:34

Russ Bradberry · Answer 2 · 2013-01-22T20:49:13.777

Part of your problem is as mjhm stated in his answer to your question, and is as you have suspected. The i variable is changing before the callback is invoked.

The other half of your problem is because your database calls have not returned yet. Due to the asynchronous nature of NodeJS, your loop will finish before your database calls complete. Additionally, your database calls are not necessarily coming back in the same order you called them. What you need is some sort of flow control like async.js. Using async.map will allow you to make all calls to the DB in parallel and return them as an array of values you can use, after all db calls have been completed.

async.map(tempgame.stats, function(stat, callback){
  db.users.find({ username: stat.user }, function(err, res){
      if( err != null){
          callback(err);
      } else {
          callback(null, res[0].stats);
      }
  });      
}, function(err, stats){
  if(err){
    //handle errors
  } else{
    stats.forEach(function(stat){
      //do something with your array of stats
      //this wont be called until all database calls have been completed
    });
  }
});

score 0 · Answer 3 · answered Sep 11 '13 at 10:26

0

In addition to the above, if you want to return results back to the application, http://nodeblog.tumblr.com/post/60922749945/nodejs-async-db-query-inside-for-loop

answered Sep 11 '13 at 10:26

Avinash

779
7
18

nodejs and mongodb (mongojs): Trying to query and update database within a for loop

3 Answers3