9
\$\begingroup\$

This script is meant to look for an id in the data attribute in each containment div, then send an Ajax call to get the amount of retweets, calculate the karma based on those retweets and then display that number in a div.

This code looks like spaghetti. How do I improve this to make each function more independent? There is some abstraction function that I can't figure out.

My other question is, how can I use a jQuery deferred object to create a promise and update the div once I get a response?

function get_retweet(id) {
    var url = 'https://api.twitter.com/1/statuses/show/' + id + '.json',
        karma; 

    $.ajax({
        url: url,
        type: 'GET',
        dataType:'jsonp',
        crossDomain: true,
        success: function(data) {
            display_karma(data.retweet_count);
        },
        error: function() {alert('fail');}
    }));
}

function calc_karma(tweets) {
    return tweets *10 +10;    
}

function display_karma(retweets) {
    var id, karma,
        el =$('.tweetContainer');
    //id = $(el).data(el, tweet_id);
    id = '248988915661410304';
    karma = calc_karma(retweets);
    el.find('.tcPoints').text(karma);
}

function start_get_karma() {
    var id;

    $('.tweetContainer').each(function(index,el) {
        //id = $(el).data(el, tweet_id);
        id = '248988915661410304';
        get_retweet(id);
    });
}
\$\endgroup\$
  • 3
    \$\begingroup\$ I don't think this code is spaghetti. \$\endgroup\$ – Florian Margaine Sep 24 '12 at 6:59
  • \$\begingroup\$ Although there is a global variable and unnecessary declaration, this code is not spaghetti. It is well separated and clearly readable. \$\endgroup\$ – Florian Margaine Sep 24 '12 at 7:01
9
\$\begingroup\$

Here are some tips:

  1. Use $.getJSON() to get JSON.

    Also, name the first parameter json for clarity.

    Note from documentation for jQuery.getJSON()

    JSONP

    If the URL includes the string "callback=?" (or similar, as defined by the server-side API), the request is treated as JSONP instead. See the discussion of the jsonp data type in $.ajax() for more details.

    More information here

    Old Code:

    var url = 'https://api.twitter.com/1/statuses/show/' + id + '.json',
        karma; 
    $.ajax({
        url: url,
        type: 'GET',
        dataType:'jsonp',
        crossDomain: true,
        success: function(data) {
            display_karma(data.retweet_count);
        },
        error: function() {alert('fail');}
    }));
    

    New Code:

    var url = 'https://api.twitter.com/1/statuses/show/' + id + '.json?callback=?';
    $.getJSON( url, function (json) {
        display_karma(json.retweet_count);
    }).error(function () {
        alert('fail');
    });
    
  2. Eliminate commented code. Use a source control system like git, svn, mercurial to keep track of changes.

    Here's a good start for it

    Old Code:

    function start_get_karma() {
        var id;
    
        $('.tweetContainer').each(function(index,el) {
            //id = $(el).data(el, tweet_id);
            id = '248988915661410304';
            get_retweet(id);
        });
    }
    

    New Code:

    function start_get_karma() {
        var id = '248988915661410304';
        $('.tweetContainer').each(function (index, el) {
            get_retweet(id);
        });
    }
    
  3. Function calls are expensive, so use a basic loop instead of each() when appropriate.

    Old Code:

    $('.tweetContainer').each(function (index, el) {
        get_retweet(id);
    });
    

    New Code A:

    for(var i = 0; len = $('.tweetContainer').length; i < len; i++){
        get_retweet(id);
    }
    

    However, there's a problem. Making multiple calls to get_retweet() with the same static value doesn't make sense. So just make one call.

    New Code B:

    if( $('.tweetContainer').length ){
        get_retweet(id);
    }
    
  4. Don't declare variables if they're only used once.

    Old Code:

    function display_karma(retweets) {
        var id, karma,
            el =$('.tweetContainer');
            id = '248988915661410304';
        karma = calc_karma(retweets);
        el.find('.tcPoints').text(karma);
    }
    

    New Code:

    function display_karma(retweets) {
        var id = '248988915661410304';
        $('.tweetContainer').find('.tcPoints').text(calc_karma(retweets));
    }
    
  5. Have variable names give a hint to the type.

    retweets sounds like a function or array. Try naming it retweet_amount or something similar.

    Final Code:

    function calc_karma(tweets) {
        return (tweets * 10) + 10;
    }
    function display_karma(retweet_amount) {
        $('.tweetContainer').find('.tcPoints').text(calc_karma(retweet_amount));
    }
    function get_retweet(id) {
        var url = 'https://api.twitter.com/1/statuses/show/' + id + '.json?callback=?';
        $.getJSON( url, function (json) {
            display_karma(json.retweet_count);
        }).error(function () {
            alert('fail');
        });
    }
    function start_get_karma() {
        var id = '248988915661410304';
        if( $('.tweetContainer').length ){
            get_retweet(id);
        }
    }
    
\$\endgroup\$
6
\$\begingroup\$

You can use jQuerys .when function. As $.ajax returns a defered/promise object you can pass that object to jquery.when. Like this.

function get_retweet(id) {
    var url = 'https://api.twitter.com/1/statuses/show/' + id + '.json',
        karma; 

    // return the defered object
    return $.ajax({
        url: url,
        type: 'GET',
        dataType:'jsonp',
        crossDomain: true,
        error: function() {alert('fail');}
    }));
}


function start_get_karma() {
    var id;

    $('.tweetContainer').each(function(index,el) {
        //id = $(el).data(el, tweet_id);
        id = '248988915661410304';
        $.when( get_retweet(id) ).then(function( data ){
            display_karma(data.retweet_count);
        })
    });
}
\$\endgroup\$
1
\$\begingroup\$

I think your problem is best solved by using some MVC framework like AngularJS. It helps you handle deferred promise and update your views automatically out of the box.

\$\endgroup\$
  • 5
    \$\begingroup\$ I think for something as small as his code any MVC-ish framework is pure overkill. \$\endgroup\$ – ThiefMaster Sep 24 '12 at 2:10
  • 1
    \$\begingroup\$ I beg to differ. MVC encourages cleaner code with clear separation between Model and View. Also, it's not fun at all to try to use a framework after your codebase get large enough for you to consider using one. \$\endgroup\$ – tanyehzheng Sep 24 '12 at 2:47
  • 3
    \$\begingroup\$ @tanyehzheng I beg to differ. If you need an MVC framework to encourage cleaner code with clear separation, you're doing something wrong. \$\endgroup\$ – Florian Margaine Sep 24 '12 at 7:00
  • 1
    \$\begingroup\$ @Cygal I'm sorry, but that's a very stupid statement. I'm repeating myself: if you need AngularJS to structure your codebase, you're doing something wrong. Seriously. \$\endgroup\$ – Florian Margaine Sep 24 '12 at 17:42
  • 1
    \$\begingroup\$ I didn't say I need it. \$\endgroup\$ – Quentin Pradet Sep 25 '12 at 6:23

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.