Javascript dont make functions inside loop

Question

Can someone please tell me whats is wrong with this code? One: I'm not getting the desired result, and two: I'm getting the famous "don't make functions inside loop". How can I fix this?

// Modify this file to make `getNames` work as described
// The tests in index.html will pass when the function is working
//
// Given a list of ids, this function should
//  - use the nameLookup api to find the name for each id
//  - call the callback argument with an object in the format
//    { 1: 'Name1', 2: 'Name2', 3: 'Name3' }
function getNames(ids, callback) {
  var index,
    id,
    results = { };

  for(index = 0; index < ids.length; index++) {
    id = ids[index];

    nameLookup.nameOf(id, function(name) {
      results[id] = name;
    });
  }

  callback(results);
}

namelookup

// Don't modify this file
//
// This is just here to fake API-like responses
// modify get-names.js instead
var nameLookup = {
  names: {},
  list: ['Adam', 'Ali', 'Alex', 'Brian', 'Cam', 'Chris'],

  // "Asynchronously" looks up the name for an id
  // Calls the callback argument with the name
  nameOf: function(id, callback) {
    var self = this;

    setTimeout(function() {
      // This just provides random results
      var index = Math.floor(Math.random() * self.list.length);
      self.names[id] = self.names[id] || self.list[index];
      callback(self.names[id]);
    }, Math.random() * 200);
  }
};

@SudhansuChoudhary — See the previous line. It is the argument name. — Quentin, Jan 23 '16 at 20:17
Well, in this case the creation of new functions does not create unexpected behaviour, so you really need not to change it. However, you could optimise the code by moving the function declaration outside of the loop and re-use the same function over and over. — Stephan Bijzitter, Jan 23 '16 at 20:18
@dooagain posted it on top, however Im not supposed to modify that part — Javier Lopez, Jan 23 '16 at 23:56

Gavriel · Answer 1 · 2016-01-23T23:25:35.760

0

So make the function before the loop:

function getFunc(id) {
  return function(name) {
      results[id] = name;
      done ++;
      if (done == ids.length) {
          callback(results);
      }
    }
}

function getNames(ids, callback) {
  var index,
    id,
    results = { };

  var done = 0;
  for(index = 0; index < ids.length; index++) {
    id = ids[index];

    nameLookup.nameOf(id, getFunc(id));
  }
}

edited Jan 23 '16 at 23:25

answered Jan 23 '16 at 20:15

Gavriel

18,880
12
68
105

The callback is still called before all names have been looked up – Prinzhorn Jan 23 '16 at 20:17
Ah, I see the problem now :) I made an edit, please check – Gavriel Jan 23 '16 at 20:24
Now you're having a race condition because every async request shares the same `id` variable. Basically you will end up with `results` only containing a single entry with the last id as key and the value of the request that returned last (which is no necessarily the last id). – Prinzhorn Jan 23 '16 at 21:16
@Prinzhorn, in other words we're back on "creating a function in each loop" to include id in the closure, aren't we? – Gavriel Jan 23 '16 at 21:27
1

I guess so. To be honest I'd have used [async.map](https://github.com/caolan/async#map) straight away, but since the question sounds like homework that's probably not allowed. – Prinzhorn Jan 23 '16 at 21:31
@Gavriel in this case the 'id' in 'results[id] = name;' is always three. How can I make that 'id' to increment as it does outside the function? – Javier Lopez Jan 23 '16 at 23:18
@Prinzhorn like you mentioned above this is whats happening to me "Basically you will end up with results only containing a single entry with the last id as key and the value of the request that returned last" – Javier Lopez Jan 24 '16 at 00:03

Javascript dont make functions inside loop

1 Answers1