current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I have the follow function:

Download.prototype.fundamentals = function(exchanges, cb){

    if (!(exchange = exchanges.pop())) return

    var that = this;

    this.login(function(err){

        if (err) return cb(true, exchange.code)

        var path = util.format(that.eoddata.fundamentals['path'], that.token, exchange.code)

        that.get(that.eoddata.fundamentals['host'], path, function(err, data){          
            if (err) return cb(true, exchange.code) 

            that.xml(data, function(err, data){             
                if (err || data['@'].Message != 'Success') return cb(true, exchange.code)               

                var stocks = data['FUNDAMENTALS']['FUNDAMENTAL']

                that.insert(stocks, exchange, function(){ 
                    cb(false, exchange.code) 
                })  

                that.fundamentals(exchanges, cb)                                                
            })                          

        })

    })  
}

I think it easy to understand, I pop() an element to a list and then do other operations, and then recall the function to pop() another element.

I have a doubt regarding the writing of the function, now it works, but it does not seem very optimized. Example: I do not like var that = this.

How could I rewrite it better?

share|improve this question

migrated from stackoverflow.com Jan 19 '12 at 1:17

This question came from our site for professional and enthusiast programmers.
1  
This might be a better fit on Code Review; I've flagged for moderator attention to migrate, we'll see if they agree you'll do better there. (You can flag too, if you want it higher up the moderator queue.) –  sarnold Jan 18 '12 at 23:24

1 Answer 1

up vote 4 down vote

Firstly, you are missing a var exchange; in your function.

Heavily nested callbacks are no fun for anyone. I would consider using the async library to try to simplify the callbacks.

https://github.com/caolan/async#waterfall

Something like this. The callbacks passed to waterfall are called in series, and any arguments passed to each callback function after the err argument, are passed as the first parameters of the next function.

var that = this is just something you kind of have to get used to. In some cases you can avoid it by using various methods to bind the value of 'this' to the proper value.

Download.prototype.fundamentals = function(exchanges, cb) {

  var that = this,
      exchange = exchanges.pop();

  if (!exchange) return;

  async.waterfall([
    function(callback) {
      that.login(callback);
    },
    function(callback) {
      var path = util.format(that.eoddata.fundamentals['path'], that.token, exchange.code)
      that.get(that.eoddata.fundamentals['host'], path, callback);
    },
    function(data, callback) {
      that.xml(data, function(err, xmldata) {
        err = err || xmldata['@'].Message != 'Success';
        callback(err, xmldata);
      });
    },
    function(xmldata, callback) {
      var stocks = xmldata['FUNDAMENTALS']['FUNDAMENTAL']
      that.insert(stocks, exchange, callback);
      that.fundamentals(exchanges, cb);
    }
  ], function(err) {
    cb(err, exchange.code);
  });
}
share|improve this answer

Your Answer

 
discard

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