2
\$\begingroup\$

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);
    });
});
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

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.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Commented May 23, 2015 at 4:21

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.