Controller making request to an external API

Question

I'm starting on a new node/express backend and really want to keep everything clean and well tested. My previous project got a bit out of hand (in terms of test coverage and highly coupled code). So I'm going slow this time :-)

To be brief, this function currently sends a request to an API that responds with JSON data. It's a time based API (live results from football games) so I'm making use of some mocking tools so I don't have to spend my sunday afternoon's testing my app !

At the moment the function only returns a JSON object. Up next I want to serialise this to a database (Mongo probably).

Here's v0.1 of the controller:

function getFixture(cb){
  var url = "http://football-api.com/api/?Action=today&APIKey=" + secrets.APIKey + "&comp_id=1204";
  async.waterfall([
    function(callback){
      request.get(url, function(err, response, body){
        if (err) return callback(err);
        if (response.statusCode === 200) {
          var parsedJSON = JSON.parse(body);
          callback(null, parsedJSON);
        } else {
          var errorMsg = "Error in response with code: " + response.statusCode;
          callback(null, errorMsg);
        }
      });
    }
  ], cb)
}

module.exports = getFixture;

I'll include my tests below however they take a lot of space so I'll keep writing here. To be frank I'm still pretty hazy with exactly what's going on with the callbacks in node. So apologies from the outset if there are any obvious errors.

I've used async.waterfall from the get-go since I anticipate having another function that will save this parsedJSON object to a database, which naturally can only occur after the request/response cycle is complete.

Some context: this function will not be matched with a route. Instead it'll be hooked up to a cron job that will run once a Friday to get the scheduled matches for the weekend. So performance isn't really an issue, more so error handling and so on.

I'm starting to ramble, here are the tests (they all pass):

/* Module dependencies */
var nock = require('nock'),
  should = require('should');

/* Local dependencies */
var getFixture = require('../../controllers/api/fixture'),
  secrets = require('../../config/secrets'),
  jsonData = require('../mockData');


describe('Todays matches :controller', function() {

  describe('request/response life cycle', function() {
    it('sends a successful request', function (done) {
      var api = nock('http://football-api.com')
                  .get('/api/?Action=today&APIKey=' + secrets.APIKey + '&comp_id=1204')
                  .reply(200, {});

      getFixture(function (err, result) {
        should.not.exist(err);
        done();
      });
    });

    it('correctly handles an error response', function (done) {
      var api = nock('http://football-api.com')
                  .get('/api/?Action=today&APIKey=' + secrets.APIKey + '&comp_id=1204')
                  .reply(500, {});

      getFixture(function (err, result) {
        result.should.equal("Error in response with code: 500");
        done();
      });
    });
  });


  describe('Parsing JSON response', function() {
    it('stores the response in result', function (done) {
      var api = nock('http://football-api.com')
                  .get('/api/?Action=today&APIKey=' + secrets.APIKey + '&comp_id=1204')
                  .reply(200, jsonData.match_array);

      getFixture(function (err, result) {
        result.should.not.be.empty;
        done();
      });
    });

    it('is done correctly (attributes match)', function (done) {
      var api = nock('http://football-api.com')
                  .get('/api/?Action=today&APIKey=' + secrets.APIKey + '&comp_id=1204')
                  .reply(200, jsonData.match_array);

      getFixture(function (err, result) {
        var expectedMatchId = jsonData.match_array.matches[0].match_id;
        var actualMatchId = result.matches[0].match_id;

        actualMatchId.should.equal(expectedMatchId);
        done();
      });
    });


  });
});

Answer 1

After running the code through JSHint, you are missing a semicolon on line 16 of the first code swatch (], cb)). The tests look good on JSHint.

I suggest making the following changes:

Remove async.waterfall in getFixture

As you said in your own words, "performance isn't really an issue". As such, I'd recommend to just remove the waterfall function to make the code cleaner and easier to maintain. You probably won't run into a noticeable speed difference anyways, if it is on cron.

Shorten callbacks

This may be more opinionated, but I would suggest in the first swatch to change your callbacks from this format:

var errorMsg = "Error in response with code: " + response.statusCode;
callback(null, errorMsg);

to this format:

callback(null, "Error in response with code: " + response.statusCode);

It is just as easy to read (if not easier) and works just the same, except without the variable instantiation.

Shorten a line

function getFixture(cb) {...
module.exports = getFixture;

Change to

module.exports = function(cb) {...

Or, even (according to choice):

module.exports = function getFixture(cb) {...

Good luck!