2
\$\begingroup\$

I see that there are quite some words that are appearing double or more times in my code.

I'm thinking there might be a more efficient way of writing this code, only I don't have the knowledge to do so.

function transitionData(data) {
    if($("div#facebook div#notice").is(":visible")) {
        $("div#facebook div#heading *").fadeOut();
        $("div#facebook div#notice").animate({"opacity":"0"}, function() {
            $("div#facebook div.wrapper > img:first-child").attr("src", "http://graph.facebook.com/"+data.id+"/picture?type=normal");
            $("div#facebook h3").html("Facebook account of <b>"+data.name+"</b>");
            $("div#facebook div#heading a").attr("href", "http://www.facebook.com/"+data.username);
            $("div#facebook tr:first-child td:first-child b").text(data.id);
            $("div#facebook tr:first-child+tr td:first-child b").text(data.first_name);
            $("div#facebook tr:first-child+tr+tr td:first-child b").text(data.last_name);
            $("div#facebook tr:first-child td:first-child+td b").text(data.gender.charAt(0).toUpperCase() + data.gender.slice(1));
            $("div#facebook tr:first-child+tr td:first-child+td b").text(data.locale);
            $("div#facebook tr:first-child+tr+tr td:first-child+td b").text(data.username);

            $("div#facebook div#notice").slideUp();
            $("div#facebook div.wrapper").slideDown();
            $("div#facebook div#heading *").fadeIn();
        });
    } else {
        $("div#facebook div#heading *, div#facebook div.wrapper").fadeOut(function() {
            $("div#facebook div.wrapper > img:first-child").attr("src", "http://graph.facebook.com/"+data.id+"/picture?type=normal");
            $("div#facebook h3").html("Facebook account of <b>"+data.name+"</b>");
            $("div#facebook div#heading a").attr("href", "http://www.facebook.com/"+data.username);
            $("div#facebook tr:first-child td:first-child b").text(data.id);
            $("div#facebook tr:first-child+tr td:first-child b").text(data.first_name);
            $("div#facebook tr:first-child+tr+tr td:first-child b").text(data.last_name);
            $("div#facebook tr:first-child td:first-child+td b").text(data.gender.charAt(0).toUpperCase() + data.gender.slice(1));
            $("div#facebook tr:first-child+tr td:first-child+td b").text(data.locale);
            $("div#facebook tr:first-child+tr+tr td:first-child+td b").text(data.username);

            $("div#facebook div#heading *, div#facebook div.wrapper").fadeIn();            
        });
    }
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ you can store and use for example $("div#facebook") in a variable \$\endgroup\$ – Daniel Ruf Mar 10 '14 at 13:08
  • \$\begingroup\$ and duplicate code parts in the if/else loop can be made as a function with the parameter data \$\endgroup\$ – Daniel Ruf Mar 10 '14 at 13:11
  • \$\begingroup\$ I've looked at this for 15 seconds and realized I would use a template engine for this \$\endgroup\$ – megawac Mar 10 '14 at 19:16
1
\$\begingroup\$

I'll take a lead from konijn, and take it a few steps further.

  • First, as konijn did, we break out the repeated parts into their own method.
  • As Daniel Ruf noted, we can save a lot of characters by caching div#facebook node as a variable, and then using .find(selector) to look for subnodes.
  • Also, we don't need to specify the HTML element type when looking for nodes, so we can go from div#facebook to just #facebook.
  • The callback called on completion of methods such as fadeOut() is called for every element that is faded out (for example), where you probably only want this to happen once. Thus, we can replace the call back with a promise that will be called only once, after all of the animation is complete.
  • I'd also give the same warning as konijn regarding the approach of using elements in a certain order:

    Finally, I must say, your approach is terrible. Your elements should have unique id's ( first_name, username etc. etc. ), the way you select elements now is a maintenance nightmare. Re-ordering the elements means re-writing the JavaScript code!

This gives us a version of the code that looks like this:

function transitionData(data) {
    var $facebook = $("#facebook");
    var $notice = $facebook.find("#notice"); 
    if($notice.is(":visible")) {
        var $headingStar = $facebook.find("#heading *");
        $headingStar.fadeOut();
        $notice.animate({"opacity":"0"}, function() {
            transitionDataDone(data, $facebook)
            $notice.slideUp();
            $facebook.find(".wrapper").slideDown();
            $headingStar.fadeIn();
        });
    } else {
        var $elementsToFade = $facebook.find("#heading *, .wrapper");
        $elementsToFade.fadeOut();
        $elementsToFade.promise().done(function() {
            transitionDataDone(data, $facebook);
            $elementsToFade.fadeIn();
        });
    }
}

function transitionDataDone(data, $facebook){
    $facebook.find(".wrapper > img:first-child").attr("src", "http://graph.facebook.com/"+data.id+"/picture?type=normal");
    $facebook.find("h3").html("Facebook account of <b>"+data.name+"</b>");
    $facebook.find("#heading a").attr("href", "http://www.facebook.com/"+data.username);
    $facebook.find("tr:first-child td:first-child b").text(data.id);
    $facebook.find("tr:first-child+tr td:first-child b").text(data.first_name);
    $facebook.find("tr:first-child+tr+tr td:first-child b").text(data.last_name);
    $facebook.find("tr:first-child td:first-child+td b").text(data.gender.charAt(0).toUpperCase() + data.gender.slice(1));
    $facebook.find("tr:first-child+tr td:first-child+td b").text(data.locale);
    $facebook.find("tr:first-child+tr+tr td:first-child+td b").text(data.username);
}
\$\endgroup\$
2
\$\begingroup\$

From a once over,

  • You have far more than a few words that are copy pasted, you could extract all the common statements into a common function:

    function transitionDataDone( data ){
        $("div#facebook div.wrapper > img:first-child").attr("src", "http://graph.facebook.com/"+data.id+"/picture?type=normal");
        $("div#facebook h3").html("Facebook account of <b>"+data.name+"</b>");
        $("div#facebook div#heading a").attr("href", "http://www.facebook.com/"+data.username);
        $("div#facebook tr:first-child td:first-child b").text(data.id);
        $("div#facebook tr:first-child+tr td:first-child b").text(data.first_name);
        $("div#facebook tr:first-child+tr+tr td:first-child b").text(data.last_name);
        $("div#facebook tr:first-child td:first-child+td b").text(data.gender.charAt(0).toUpperCase() + data.gender.slice(1));
        $("div#facebook tr:first-child+tr td:first-child+td b").text(data.locale);
        $("div#facebook tr:first-child+tr+tr td:first-child+td b").text(data.username);
    }
    
  • Once you have done that, you will notice that you are not repeating too many jQuery selectors. The only one that stands out to me is $("div#facebook div#notice"); If you cache this selector and apply the previous code, you get something like this (not tested, but you should get the gist):

    function transitionData(data) {
        var $divNotice = $("div#facebook div#notice"); 
        if($divNotice.is(":visible")) {
            $("div#facebook div#heading *").fadeOut();
            $divNotice.animate({"opacity":"0"}, function() {
                transitionDataDone( data )
                $divNotice.slideUp();
                $("div#facebook div.wrapper").slideDown();
                $("div#facebook div#heading *").fadeIn();
            });
        } else {
            $("div#facebook div#heading *, div#facebook div.wrapper").fadeOut(function() {
                transitionDataDone( data );
                $("div#facebook div#heading *, div#facebook div.wrapper").fadeIn();            
            });
        }
    }
    
    function transitionDataDone( data ){
        $("div#facebook div.wrapper > img:first-child").attr("src", "http://graph.facebook.com/"+data.id+"/picture?type=normal");
        $("div#facebook h3").html("Facebook account of <b>"+data.name+"</b>");
        $("div#facebook div#heading a").attr("href", "http://www.facebook.com/"+data.username);
        $("div#facebook tr:first-child td:first-child b").text(data.id);
        $("div#facebook tr:first-child+tr td:first-child b").text(data.first_name);
        $("div#facebook tr:first-child+tr+tr td:first-child b").text(data.last_name);
        $("div#facebook tr:first-child td:first-child+td b").text(data.gender.charAt(0).toUpperCase() + data.gender.slice(1));
        $("div#facebook tr:first-child+tr td:first-child+td b").text(data.locale);
        $("div#facebook tr:first-child+tr+tr td:first-child+td b").text(data.username);
    }
    
  • Finally, I must say, your approach is terrible. Your elements should have unique id's ( first_name, username etc. etc. ), the way you select elements now is a maintenance nightmare. Re-ordering the elements means re-writing the JavaScript code!
\$\endgroup\$
1
  • 1
    \$\begingroup\$ totally right, unique CSS IDs are much faster and better to select \$\endgroup\$ – Daniel Ruf Mar 10 '14 at 17: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.