2
\$\begingroup\$

I'm looking for feedback as to how I've structured the async calls, and If anything should be written differently.

endpoints.js

module.exports = {
  lotto: 'https://www.norsk-tipping.no/api-lotto/getResultInfo.json?drawID=',
  keno: 'https://www.norsk-tipping.no/api-keno/getResultInfo.json?drawID=',
  extra: 'https://www.norsk-tipping.no/api-extra/getResultInfo.json?drawID=',
  vikinglotto: 'https://www.norsk-tipping.no/api-vikinglotto/getResultInfo.json?drawID=',
  joker: 'https://www.norsk-tipping.no/api-joker/getResultInfo.json?drawID=',
  eurojackpot: 'https://www.norsk-tipping.no/api-eurojackpot/getResultInfo.json?drawID='
};

service.js

var request      = require('request');
var gametypes    = require('./endpoints');

function toJSON(response){
    var data    = response.toString(),
        result  = data.match(/(^{[\s\w\W]+}$)/gm).join('');

    return JSON.parse(result);
}


function getResults(opts, callback){
    var url                     = gametypes[opts.type],    
        fromDrawID              = opts.fromDrawID,
        toDrawID                = opts.toDrawID,
        numberOfRequestsWanted  = toDrawID - fromDrawID,
        numberOfRequestsDone    = 0,
        data                    = [];

    //If either the object is missing or
    //the object with no key of 'type' is passed in - return error. 

    if(!opts.type){
        return callback(new Error('Missing required param: Object with key: type'), null);
    }


    if(fromDrawID && toDrawID) {
        console.log('Fetching the requested results from https://www.norsk-tipping.no...');

        while(fromDrawID <= toDrawID) {
            doRequest(url + fromDrawID++, callback);
        }
    } else {
        console.log('Fetching the latest result from https://www.norsk-tipping.no...');
        doRequest(url, callback);
    }

    function doRequest(url, callback) {

        request(url, function(error, response, body) {
            if(!error && response.statusCode === 200) {
                data.push(toJSON(body));

                if(numberOfRequestsDone++ === numberOfRequestsWanted) {
                    return callback(null, data);
                } 

            } else {
                return callback(error, null);
            }
        });
    }
}

module.exports = getResults;

example usage

var lotto    = require('./service');
var options  = { type: 'vikinglotto', fromDrawID: 1, toDrawID: 500};


lotto(options, function(err, results) {
    if (err) console.log(err);

    else {        
        console.log(data);
    }
});
\$\endgroup\$
  • \$\begingroup\$ Hello! Please don't make changes to the original post once it has been reviewed, as that invalidates the current answers. Please see our meta side on performing iterative reviews for more information! \$\endgroup\$ – syb0rg Jun 28 '16 at 16:35
  • \$\begingroup\$ @syb0rg I didn't change any of the original content - just added to it. But if that's not allowed - no problem. \$\endgroup\$ – Nilzone- Jun 28 '16 at 16:38
1
\$\begingroup\$

Suggesting you move to using promises instead of callbacks. This way, you don't have to do the tango with calling callbacks, collecting results and all. You can simply replace the request module with the request-promise module.

function getResult(options){
  ...
  return doRequest(options);
}

...

lotto(options).then(results => console.log(data), err => console.log(err));

There's little merit of putting doRequest inside getResults. It's only being used in getResults but it's not exposed anyways. Suggesting you move it out into the module.

JavaScript doesn't have default argument values (yet) but you can easily do the same thing using Object.assign. Provide a default object, and merge to a new object the defaults and the one from the arguments. This way, you avoid having to do a lot of default logic and value checks.

var defaults = {...}

function getResults(options){
  var mergedOptions = Object.assign({}, defaults, options);
  ...
}

Back to promises, firing multiple async calls in parallel using callbacks will require you to collect results and keep checking if all the requests have responded. When using Promises, you can easily use Promise.all to listen to an array of promises.

var request = require('request-promise');

function getResult(options){
  ...
  var drawRange = toDrawID - fromDrawID;

  // Create an array of drawIds and map them to promises
  var promises = Array(drawRange).fill(fromDrawID).map((fromDrawID, i) => {
    return request(url + (fromDrawID + i));
  });

  return Promise.all(promises);
}
\$\endgroup\$
  • \$\begingroup\$ Thanks! What did you mean by "...Suggesting you move it out into the module"? The doRequests is used twice inside the getResults. Isn't it logical to place the function there since that's the only place I use it? \$\endgroup\$ – Nilzone- Jun 27 '16 at 16:51
  • \$\begingroup\$ @Nilzone- You wouldn't want to redeclare functions on every call. \$\endgroup\$ – Joseph Jun 27 '16 at 22:21
  • 1
    \$\begingroup\$ One thing - the fill-function don't work the way you intended to. The array is just filled with the same id (never incremented). I had to do this instead of fill Object.keys([...Array(numberOfRequestsWanted)]) \$\endgroup\$ – Nilzone- Jun 28 '16 at 15:57
  • 1
    \$\begingroup\$ @Nilzone- Ahh, my mistake. However, you can fill in fromDrawID for all items and use array.map's second argument (a zero-based index) to add to the ID. \$\endgroup\$ – Joseph Jun 28 '16 at 16:14
  • \$\begingroup\$ But I would also like to parse the response to proper json before I actually return Promise.all. I updated my question - is this a proper way to do it? \$\endgroup\$ – Nilzone- Jun 28 '16 at 16:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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