0

I am trying to implement a for loop that iterates through a list and subsequently calls two functions, only if the first function results are found.

The issue is that the second function (search.similar) might be taking longer to fetch results.

With the code below, when I run, all of the appropriate output from (search.locate) is correct, but only the last element's results from myList are stored from the (search.similar) function.

ie. all_results = [[cat_res1,mouse_res2],[dog_res1,mouse_res2],[mouse_res1,mouse_res2]]

How do I fix this to append the right results in the right order?

ie. all_results = [[cat_res1,cat_res2],[dog_res1,dog_res2],[mouse_res1,mouse_res2]]

var search = require('./search');
var myList = ['cat','dog','mouse'];
var all_results = [];
for (i=0; i<myList.length; i++){
  /* locate function*/
  search.locate(myList[i], function (err, searchResult){
    if (err){
      console.log("Error");
      return;
    }

    if (!searchResult){
      console.log("Cannot find it");
      return;
    }

    /*similarity function*/
    /* seems to take longer*/
    search.similar(myList[i], function (err, similarResult){
      if (err){
        return;
      }

      if (!similarResult){
        return;
      }

      var res1 = searchResult.data;
      var res2 = similarResult.data;
      /* append results to array*/
      all_results.push([res1,res2]);
    }
  });
}
user7771338
  • 185
  • 6
chattrat423
  • 603
  • 2
  • 11
  • 24
  • What type of value is returned by *search.locate* and *search.similar*? Are the methods asynchronous? – RobG Mar 28 '17 at 23:08
  • Don't use `push` but assign to the respective index that you store in a [properly scoped](http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example) variable. – Bergi Mar 28 '17 at 23:10
  • @RobG let's assume they will be objects of some kind – chattrat423 Mar 28 '17 at 23:26
  • @Bergi, do you mean like this > all_results[i] = [res1,res2] – chattrat423 Mar 28 '17 at 23:27
  • If they're objects, then you may be assigning references to the same object. But it appears (from other comments) that the calls may be asynchronous and closures are affecting the results. – RobG Mar 29 '17 at 00:50

2 Answers2

0

Javascript can be thought of as asynchronous, in that the execution of particular functions do not necessarily happen synchronously, however "describing JavaScript as asynchronous is perhaps misleading. It's more accurate to say that JavaScript is synchronous and single-threaded with various callback mechanisms"

In order to accomplish your goal, though you may still get some ordering issues with the top array, you will need to wrap your .similar() call in another function that takes both arguments. Your reference to the "item" on the top search is changing:

function searchNestedSimilar(item, topRes) {
    search.similar(item, function (err, similarResult) {

        if (err){
            return;
        }
        if (!topRes){
            return;
        }

        var res1 = topRes.data
        var res2 = similarResult.data

        // append results to array
        all_results.push([res1,res2])

    }
} 

function searchLocate(item) {
   search.locate(item, function (err, searchResult) {

    if (err){
        console.log("Error");
        return;
    }
    if (!searchResult){
        console.log("Cannot find it");
        return;
    }
    searchNestedSimilar(item, searchResults);
}

I encapsulated both calls to keep it modular, but since "item" is in the closure, you really only need the searchLocate() function to wrap your capture your item reference during iteration.

Steve Harris
  • 306
  • 2
  • 4
  • Yes, this code still has the ordering issues that the question asked to resolve. – Bergi Mar 28 '17 at 23:13
  • Not sure what you mean by "*Your reference on the top search*"? – Bergi Mar 28 '17 at 23:14
  • "*Javascript is asynchronous*"—not in general. – RobG Mar 28 '17 at 23:15
  • Alas, it's true, issues remain. Both asynchronous calls need to be wrapped as myList[i] isn't parameterized either. I have fixed my example. – Steve Harris Mar 28 '17 at 23:17
  • @RobG http://stackoverflow.com/questions/2035645/when-is-javascript-synchronous I do still think of it as aynchronous - because in browsers things get triggered by events, and in node.js there is still an event loop. Thinking of things as synchronous gets one into trouble, as chattrat423 has discovered. But it is definitely single-threaded, so consideration has to be made for long running loops, etc. – Steve Harris Mar 28 '17 at 23:21
  • Ah, you meant the `item[i]` reference in the `search.locate` callback – Bergi Mar 28 '17 at 23:29
  • @Bergi `myList[i]`, yes. Common problem with iterators and asynch functions is that they just zip through the loop and the index or iterated item keeps changing. Another problem with loops is that they can run long, especially long loops. Node.js now has a setImmediate(cb) function that I have found useful, server side, for this. It almost does the same thing as setTimeout(cb,0) – Steve Harris Mar 28 '17 at 23:34
  • @SteveHarris—there are also promises and web workers (which didn't exist back in 2010 when that answer was written). ;-) Anyway, it seems the OP's issue may be that the calls are asynchronous. – RobG Mar 29 '17 at 00:55
  • @SteveHarris, not sure where/what the topRes piece is doing? – chattrat423 Mar 29 '17 at 00:59
  • @SteveHarris, when I run it, it seems like searchLocate(item) doesn't execute? – chattrat423 Mar 29 '17 at 01:04
0

This is a good case for Promises (see Bluebird JS for example http://bluebirdjs.com/docs/getting-started.html) or you could do it with async.map().

This page talks about it well, too. http://promise-nuggets.github.io/articles/14-map-in-parallel.html

There are many Stack Overflows discussing Promises as well. Understanding JS Promises for example.

A rough example of how to write this with a Promise:

var search = require('./search');
var myList = ['cat','dog','mouse']
var all_results = []
var Promise = require('bluebird');
var locate = Promise.promisify(search.locate);
var similar = Promise.promisify(search.similar);

for (i = 0; i < myList.length; i++){
    // locate function
    locate(myList[i], function (err, searchResult) {
        if (err) {
            console.log("Error");
            return;
        }
        if (!searchResult){
            console.log("Cannot find it");
            return;
        }
    }).then(function(result) {
        //similarity function
        similar(myList[i], function (err, similarResult) {
            if (err){
                return;
            }
            if (!similarResult){
                return;
            }

            var res1 = searchResult.data
            var res2 = similarResult.data

            // append results to array
            all_results.push([res1,res2])
        }).finally(function() {
            // NOP
        });
    });
}
Community
  • 1
  • 1
Misha Nasledov
  • 2,003
  • 1
  • 15
  • 18