Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I would like someone review this code and tell if it can be done better, or if the code is elegant and simple enough for the task at hand.

I used code climate and I got 4/4 and my test coverage is 96%, yet I would like a professional opinion about it.

'use strict';

var PromisePolyfill = require('promise');
var http = require('http');

var twitterCountURL = 'http://urls.api.twitter.com/1/urls/count.json?url=';

/**
 * Gets the numbers of tweets of a given url.
 * @param {string} validUrl - The url to query
 * @returns {Number} Number of tweets
*/
exports.getCount = function(validUrl) {
    return new PromisePolyfill(function(resolve, reject) {
        if (validUrl) {
            http.get(twitterCountURL + validUrl, function(res) {
                var body = '';
                res.on('data', function(chunk) {
                    body += chunk;
                });
                res.on('end', function() {
                    var twResponse = JSON.parse(body);
                    if (!twResponse.count) {
                        twResponse.count = 0;
                    }
                    // console.log(twResponse.count);
                    resolve(twResponse.count);
                });

            }).on('error', function(e) {
                reject(e.message);
            });

        }else {
            resolve(0);
        }
    });
};

And its respective test file:

'use strict';

var chai = require('chai');
var chaiAsPromised = require('chai-as-promised');
var sinon = require('sinon');
var http = require('http');
var PassThrough = require('stream').PassThrough;
var twitterCount = require('../../modules/twitterCount');

var response,
  responsePT;

chai.should();
chai.use(chaiAsPromised);

describe('twitterCount Module', function() {

    beforeEach(function(done) {
        response = new PassThrough();
        responsePT = new PassThrough();
        sinon.stub(http, 'get');
        done();
    });

    afterEach(function(done) {
        http.get.restore();
        done();
    });

    it('should return valid count when a valid url is passed', function(done) {
        var expected = {'count': 234453234, 'url': 'http:\/\/www.google.com\/'};

        response.write(JSON.stringify(expected));
        response.end();

        http.get.callsArgWith(1, response)
                .returns(responsePT);

        twitterCount.getCount('http://www.google.com').should.eventually.equal(234453234).notify(done);
    });

    it('should return 0 when a invalid url is passed', function(done) {
        var expected = {'count': 0, 'url': 'http:\/\/invalidurl.invalid\/'};

        response.write(JSON.stringify(expected));
        response.end();

        http.get.callsArgWith(1, response)
                .returns(responsePT);

        twitterCount.getCount('http://invalidurl.invalid').should.eventually.equal(0).notify(done);
    });

    it('should return 0 when an empty url is passed', function(done) {
        var expected = {'count': 0, 'url': ''};

        response.write(JSON.stringify(expected));
        response.end();

        http.get.callsArgWith(1, response)
                .returns(responsePT);

        twitterCount.getCount('').should.eventually.equal(0).notify(done);
    });
});
share|improve this question
up vote 1 down vote accepted

JSON.parse must always be wrapped in a try catch, because you cannot guarantee that the body will be in proper format. This will crash your server on failure.

You should add a test case to confirm this, just by responding with an empty string.

share|improve this answer
1  
Thanks @duane, I added the try / catch and added the test as well to the github repo. If there is anything else, please let me know. I appreciate your review. – Andres Osorio May 23 '15 at 4:21

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.