How to collect the value to an array in Nodejs loop?

Question

I wrote the code below, trying to collect the videos in an array then return. The code is wrong But I can't figure out the right way to do this.

var redis = require('redis');
var client = redis.createClient();

app.get('/topvideos', function(req, res){
  res.type('application/json');
  var topvideos = [];

  client.hkeys("topvideos", function(err,replies) {
    console.log("Results for video:");
    console.log(replies.length + " videos:");

    replies.forEach(function (reply, i) {

      client.hget("topvideos",i, function (err, reply) {
        console.log(i + ": " + reply );
        topvideos.push(reply);
      });
    });

  }
  var string = JSON.stringify(topvideos)
  res.send(string);
});

Is there an elegant pattern I could follow?

possible duplicate of [Why is my variable unaltered after I modify it inside of a function? - Asynchronous code reference](http://stackoverflow.com/questions/23667086/why-is-my-variable-unaltered-after-i-modify-it-inside-of-a-function-asynchron) — Scimonster, Nov 25 '14 at 05:21
@Scimonster - this is a similar issue to that other question, but that answer doesn't really tell them how to fix this more complicated scenario. — jfriend00, Nov 25 '14 at 05:25
do you looking for [hgetall](http://redis.io/commands/hgetall)? — vp_arth, Nov 25 '14 at 05:30

jfriend00 · Accepted Answer · 2014-11-25T05:30:30.667

Presumably the .hkeys method is asynchronous. That means you have to write code that knows when all async items are done so you can then (and only then) to your final res.send() with the accumulate of results.

There are many ways to keep track of when all the async operations are complete. My favorite is to promisfy all the involved functions and use Promise.all(). But, since you aren't using promises in this code yet, here's a method using a manual counter. Increment the counter whenever you start an async task. Decrement the counter whenever you finish an async task. When the counter hits zero, all async operations are done:

var redis = require('redis');
var client = redis.createClient();

app.get('/topvideos', function(req, res){
  res.type('application/json');
  var topvideos = [];
  var cntRemaining = 0;

  client.hkeys("topvideos", function(err,replies) {
    console.log("Results for video:");
    console.log(replies.length + " videos:");

    replies.forEach(function (reply, i) {
      ++cntRemaining;

      client.hget("topvideos",i, function (err, reply) {
        console.log(i + ": " + reply );
        topvideos.push(reply);
        --cntRemaining;
        if (cntRemaining === 0) {
           res.send(JSON.stringify(topvideos));
        }
      });
    });

  }
});

vp_arth · Answer 2 · 2014-11-25T07:08:06.730

0

You can use hgetall

client.hgetall("topvideos", function(err, videos){
  var keys = Object.keys(videos);
  res.send(JSON.stringify(keys.map(function(key){return videos[key];})));
});

Documentation

Also, I recommend to wrap all separate tasks with separate functions, like:

var async = require('async');

function getKeyValueMap(obj) {
   return function(key, next) {
     return next(null, obj[key]);
   }
}

function getValues(obj, next) {
    async.map(Object.keys(obj), getKeyValueMap(obj), next);
}

function getVideoList(next) {
  client.hgetall("topvideos", function(err, videos){
    if (err) return next(null, []);
    getValues(videos, next);
  });
}

function response(err, data) {
  if (err) return res.send(err.message)
  return res.send(JSON.stringify(data));
}

getVideoList(response);

edited Nov 25 '14 at 07:08

answered Nov 25 '14 at 05:32

vp_arth

14,461
4
37
66

Still doesn't solve all of the OP's issue by itself. `res.send()` has to be moved. – jfriend00 Nov 25 '14 at 05:33
really? Why? he get hash keys, then get hash fields field-by-field. This exactly, what `hgetall` does – vp_arth Nov 25 '14 at 05:35
Assuming `hgetall()` is async, `res.send()` has to be moved inside the `.hgetall()` callback or `res.send()` will get called before `hgetall()` has done it's work and always have no results. – jfriend00 Nov 25 '14 at 05:36
Using `next` for two separate purposes in nested scopes in your second code block is horribly confusing. Please pick a different variable name for one of them. – jfriend00 Nov 25 '14 at 05:58
Then, pick a different name for the argument in `getVideoList()`. Having both be `next` made it very confusing for me to follow how your code worked. You can make it `done` or `callback`. I'm just giving you feedback that the code you wrote is hard to understand and follow because of this issue and it need not be that way. I'm suggesting an improvement to the code. – jfriend00 Nov 25 '14 at 06:17
You have a reference to `next` on two consecutive lines and they are not the same thing. That's real, real subtle and not easy to understand. I guess that's the way you like to write code. It's hard to understand and easy to get confused. I endeavor to write code that is easy to understand. I guess that isn't a concern of yours. – jfriend00 Nov 25 '14 at 06:27
I change my answer to code without this problem at all. – vp_arth Nov 25 '14 at 07:09

How to collect the value to an array in Nodejs loop?

2 Answers2

Linked