Struggling with asynchronous Node JS API calls

Question

I have just started learning Node.js and I am attempting a project which queries both the Factual API and Google maps API. I have been putting code together from several different sources and it is becoming a mess of callbacks. Also, it currently uses a for loop which means the asynchronous API call within the loop never gets run. I would be very grateful if someone could assist me with refactoring this code and also show me how to get the HTTP request to run within the loop.

Here is my current code where I get the error "TypeError: Cannot read property 'legs' of undefined":

var auth = require('./auth');
var Factual = require('./node_modules/factual-api');
var factual = new Factual(auth.key, auth.secret);
var path = require('path');
var express = require('express');
app = express();

app.configure(function() {
    app.use(express.static(path.join(__dirname, 'public')));
});

var http = require("http");

app.get('/', function(req, res) {
    var placedata = "<link rel='stylesheet' type='text/css' href='default.css' /> <table>";
    factual.get('/t/restaurants-gb', {
        limit: 50,
        sort: "$distance:asc",
        geo: {
            "$circle": {
                "$center": [req.query.lat, req.query.lng],
                "$meters": 15000
            }
        }
    }, function(error, result) {


        for (var i = 0; i < result.data.length; i++) {
            var d = new Date();
            var seconds = (d.getTime() / 1000).toFixed(0);
            url = "http://maps.googleapis.com/maps/api/directions/json?mode=transit&origin=" + req.query.lat + "," + req.query.lng + "&destination=" + result.data[i].latitude + "," + result.data[i].longitude + "&sensor=false&departure_time=" + seconds;
            var request = http.get(url, function(response) { 
                var buffer = "",
                    data, route;

                response.on("data", function(chunk) {
                    console.log(buffer);
                    buffer += chunk;
                });

                response.on("end", function(err) {
                    data = JSON.parse(buffer);
                    route = data.routes[0];

                    console.log("Time: " + route.legs[0].duration.text);
                    placedata += "<tr><th align='left'>" + result.data[i].name + "</th><td>" + result.data[i].address + "</td><td>" + result.data[i].tel + "</td><td>" + result.data[i].$distance + " Metres" + "</td></tr>";
                });
            });
        }
        placedata += "</table>";
        res.send(placedata);
        console.log(">> Home");
    });

});

app.listen(process.env.PORT, process.env.IP, function() {
    console.log('Server is running');
});

Answer 1

The error you're getting is coming from the google maps API rate limiter. If you look at the results you're getting back, you're likely getting something like this.

{ routes: [], status: 'OVER_QUERY_LIMIT' }

You should check to make sure an element in an array exists before you access it to prevent this crash.

First Refactor Pass

var auth = require('./auth');
var Factual = require('factual-api');
var factual = new Factual(auth.key, auth.secret);
var path = require('path');
var express = require('express');
var concat = require('concat-stream');
var http = require("http");
var async = require("async");

app = express();

app.configure(function() {
    app.use(express.static(path.join(__dirname, 'public')));
});


function getDirections(data, fn) {
    var url = "http://maps.googleapis.com/maps/api/directions/json?mode=transit&origin=" + data.originLat + "," + data.originLong + "&destination=" + data.destLat + "," + data.destLong + "&sensor=false&departure_time=" + data.time;
    http.get(url, function(response) { 
        response.pipe(concat(function(results) {
            fn(JSON.parse(results));
        })
    )});
}

app.get('/', function(req, res) {
    res.write("<table>");
    factual.get('/t/restaurants-gb', {
        limit: 10,
        sort: "$distance:asc",
        geo: {
            "$circle": {
                "$center": [req.query.lat, req.query.lng],
                "$meters": 15000
            }
        }
    }, function(err, result) {
        async.eachSeries(result.data, function(data, fn) {
            var d = new Date();
            var seconds = (d.getTime() / 1000).toFixed(0);
            var directionData = { originLat: req.query.lat, originLong: req.query.lng, destLat: data.latitude, destLong: data.longitude, time: seconds };

            function writeEntry(directions) {
                console.log(directions);
                if(directions.routes.length == 0) {
                    setTimeout(function() {
                        getDirections(directionData, writeEntry);
                    }, 500);
                    return;
                }
                route = directions.routes[0];
                res.write("<tr><th align='left'>" + data.name + "</th><td>" + data.address + "</td><td>" + data.tel + "</td><td>" + data.$distance + " Metres" + "</td><td>" + route.legs[0].duration.text + "</td></tr>");
                fn();            
            }

            getDirections(directionData, writeEntry);
        }, function(err) {
            res.end("</table>");    
        });
    });
});

app.listen(process.env.PORT, process.env.IP, function() {
    console.log('Server is running');
});

I've made a few major changes here.

1. First of all I got rid of the manual buffering of the google api response by using concat-stream. You point a stream at it, and once the stream is through writing it passes you the complete response.

2. I changed the output from buffering (using placedata) to writing out directly as soon as you have the data. This means the page will start showing up much more quickly instead of waiting for all of the results to come back and sending it at once.

3. I replaced the for loop with async.eachSeries. This removed a number of bugs (closure bug, writing the end of the table before the rows, etc) and simplified the data access.

4. Check to see if the google maps call failed. If it did, try it again in 500ms.

I would not consider this to be the final version. There is still a lot wrong with this code, but it at least gets you closer to what you're trying to do. You should get a developer key to use with Google Maps. This should enable you to make calls more quickly without an error.

Answer 2

Here is an (untested) refactor. I am using the async library, specifically async.map.

async docs

Now the loop which sends Google Maps requests has a callback. With a traditional for loop or Array.forEach, there's no async callback, thus you would have to roll your own way to do something later when your asynchronous tasks complete.

Other than replacing the for loop with async.map, the biggest thing I did was split some anonymous functions into named functions, to help reduce the callback hell. All the important bits are hidden inside getRestsPlusRoutes.

Compare the length of app.get(). It's easier to understand what's happening because you're only concerned with a single level of callbacks before sending a response.

I also separated the presentation logic (creating the HTML tags) from the data logic (the Factual and Google Maps API calls.) Now the code to get the restaurants and routes calls back with the data, so you can repurpose the function it at other places in your project.

I reindented to two spaces, personal preference to see more nesting.

I renamed the factual results.data to rests, so the more descriptive variable name helps document the code.

var async = require('async');
var auth = require('./auth');
var Factual = require('./node_modules/factual-api');
var factual = new Factual(auth.key, auth.secret);
var path = require('path');
var express = require('express');
app = express();

app.configure(function() {
  app.use(express.static(path.join(__dirname, 'public')));
});

var http = require("http");

app.get('/', function(req, res) {
  getRestsPlusRoutes(req, function(err, restsPlusRoutes){
    if(err){ 
      console.log(err);
      return res.send(500);
    };
    var pd = buildPlaceData(restsPlusRoutes);
    res.send(pd);
    console.log(">> Home");
  });
});


// You can mitigate callback hell by declaring your asynchronous functions
// outside the code which calls them
function getRestsPlusRoutes(req, callback){
  // we have to pass in req so we have access to req.query.lat etc
  factual.get(
  '/t/restaurants-gb', 
  {
    limit: 50,
    sort: "$distance:asc",
    geo: {
      "$circle": {
        "$center": [req.query.lat, req.query.lng],
        "$meters": 15000,
      },
    },
  }, 
  function(error, result) {
    var rests = result.data;
    async.mapLimit(
    rests,               // For each item in rests,
    1,                   // with max 1 concurrency,
    getDirections,       // call getDirections(rest, done)
    function(err, mappedAll){
      // this callback executed upon all getDirections done(null, mappedOne)
      // or on any getDirections done(err)
      // mappedAll is an array of JSON.parsed bodys from Google Maps API call
      if(err){
        return callback(err);
      };
      for(var i=0; i<rests.length; i++){
        // Attach the `.routes` property of the google map result 
        // to the factual restaurant object
        rests[i].routes = mappedAll[i].routes;
      };
      return callback(null, rests);
    }); // end of async.mapLimit function call, including inline callback declaration
  });

  // declare the iterator function within getRestsPlusRoutes 
  // to ensure `var url = ...` can access `req`
  function getDirections(rest, done){
    var d = new Date();
    var seconds = (d.getTime() / 1000).toFixed(0);
    var url = "http://maps.googleapis.com/maps/api/directions/json?mode=transit&origin=" + req.query.lat + "," + req.query.lng + "&destination=" + rest.latitude + "," + rest.longitude + "&sensor=false&departure_time=" + seconds;

    http.get(url, function(response) { 
      var buffer = "",
          data, route;

      response.on("data", function(chunk) {
        console.log(buffer);
        buffer += chunk;
      });

      response.on("end", function(err) {
        if(err){ return done(err) };
        data = JSON.parse(buffer);
        done(null, data);
        // for simplicity, the entire parsed response is passed to the iterator done()
      });
    });
  }; // end of getDirections iterator function

}; // end of getRestsPlusRoutes


function buildPlaceData(restsPlusRoutes){
  var placedata = "<link rel='stylesheet' type='text/css' href='default.css' /> <table>";

  for(var i=0; i < restsPlusRoutes.length; i++){
    var rest = restsPlusRoutes[i];
    var route = rest.routes[0];
    console.log("Time: " + route.legs[0].duration.text);
    placedata += "<tr><th align='left'>" + rest.name + "</th><td>" + rest.address + "</td><td>" + rest.tel + "</td><td>" + rest.$distance + " Metres" + "</td></tr>";
  };

  placedata += "</table>";
  return placedata;
};


app.listen(process.env.PORT, process.env.IP, function() {
  console.log('Server is running');
});