1

I've got a problem with redis and nodejs. I have to loop through a list of phone numbers, and check if this number is present in my redis database. Here is my code : 

function getContactList(contacts, callback) {
  var contactList = {};

  for(var i = 0; i < contacts.length; i++) {
    var phoneNumber = contacts[i];

    if(utils.isValidNumber(phoneNumber)) {

      db.client().get(phoneNumber).then(function(reply) {
        console.log("before");
        contactList[phoneNumber] = reply;
      });
    }
  }

  console.log("after");
  callback(contactList);

};

The "after" console log appears before the "before" console log, and the callback always return an empty contactList. This is because requests to redis are asynchronous if I understood well. But the thing is I don't know how to make it works. How can I do ?

pchmn
  • 524
  • 1
  • 5
  • 16
  • 1
    Consider refactoring your NodeJS code to use [Promises](http://www.mattgreer.org/articles/promises-in-wicked-detail/). Bluebird is an excellent choice: http://bluebirdjs.com/docs/working-with-callbacks.html – paulsm4 May 29 '16 at 20:47

4 Answers4

4

You have two main issues.

  1. Your phoneNumber variable will not be what you want it to be. That can be fixed by changing to a .forEach() or .map() iteration of your array because that will create a local function scope for the current variable.

  2. You have create a way to know when all the async operations are done. There are lots of duplicate questions/answers that show how to do that. You probably want to use Promise.all().

I'd suggest this solution that leverages the promises you already have:

function getContactList(contacts) {
    var contactList = {};
    return Promise.all(contacts.filter(utils.isValidNumber).map(function(phoneNumber) {
        return db.client().get(phoneNumber).then(function(reply) {
            // build custom object
            constactList[phoneNumber] = reply;
        });
    })).then(function() {
        // make contactList be the resolve value
        return contactList;
    });
}

getContactList.then(function(contactList) {
    // use the contactList here
}, funtion(err) {
    // process errors here
});

Here's how this works:

  1. Call contacts.filter(utils.isValidNumber) to filter the array to only valid numbers.
  2. Call .map() to iterate through that filtered array
  3. return db.client().get(phoneNumber) from the .map() callback to create an array of promises.
  4. After getting the data for the phone number, add that data to your custom contactList object (this is essentially a side effect of the .map() loop.
  5. Use Promise.all() on the returned array of promises to know when they are all done.
  6. Make the contactList object we built up be the resolve value of the returned promise.
  7. Then, to call it just use the returned promise with .then() to get the final result. No need to add a callback argument when you already have a promise that you can just return.
jfriend00
  • 683,504
  • 96
  • 985
  • 979
1

The simplest solution may be to use MGET with a list of phone numbers and put the callback in the 'then' section.

You could also put the promises in an array and use Promise.all().

At some point you might want your function to return a promise rather than with callback, just to stay consistent.

Jason Livesay
  • 6,317
  • 3
  • 25
  • 31
1

Consider refactoring your NodeJS code to use Promises.

Bluebird is an excellent choice: http://bluebirdjs.com/docs/working-with-callbacks.html

paulsm4
  • 114,292
  • 17
  • 138
  • 190
  • 1
    No particular need for Bluebird here (though I am a fan for other reasons). And, the core async oepration already uses promises here so it's just a matter of actually using the promises already present (see my answer for implementation). – jfriend00 May 29 '16 at 20:51
  • 1
    You're absolutely correct. The part about "fan for other reasons" - and the fact that there are pretty good tutorials and documentation available - is why I suggested Bluebird. But you're correct: a simple ".then" should suffice :) – paulsm4 May 29 '16 at 20:52
0

you put async code into a for loop (sync operations). So, each iteration of the for loop is not waiting for the db.client(...) function to end.

Take a look at this stackoverflow answer, it explains how to make async loops :

Here

Community
  • 1
  • 1
boehm_s
  • 5,254
  • 4
  • 30
  • 44