1
\$\begingroup\$

I been learning web development for 1.5 month now, and I am trying to improve my code. I can achieve what I want, but I think my code is really bad.

For example, I have a bunch of jQuery animations and a bunch of function to 'stop' those animations when another animation is starting. The code seems very inelegant, so how should I improve it? I been learning how to write a jQuery plugin, but I don't think it will help in this case.

$(document).ready(function(){

    $("#diagonalLine").show(500);
    $("#line").show(1300);
    $("#start").show(1500);

    $("#centerButton").click(function(){
        $("#diagonalLine").hide(1500);
        $("#line").hide(1300);
        $("#start").hide(500);
        fadeInWaterloo();
        fadeInToronto();
        fadeInTop();
        $("#selectLine").animate({width:'toggle'},1250);;
        $("#diagonalSelectLine").show(1450);
        $("#select").show(1600);    

    });

    $("#waterloo7").mouseenter(function(){
        fadeInTorontoStop();
        fadeInTopStop();
        fadeOutToronto();
        fadeOutTop();

    });
    $("#toronto5").mouseenter(function(){
        fadeInWaterlooStop();
        fadeInTopStop();
        fadeOutWaterloo();
        fadeOutTop();
        //$("#flip").show(1000);
        //$("#infoBox1").slideDown(2000);
    });
    $("#campus").mouseenter(function(){
        fadeInWaterlooStop();
        fadeInTorontoStop();
        fadeOutWaterloo();
        fadeOutToronto();
    });

    $("#waterloo7").mouseleave(function(){
        fadeOutTorontoStop();
        fadeOutTopStop();
        fadeInToronto();
        fadeInTop();

    });     

    $("#toronto5").mouseleave(function(){
        fadeInWaterloo();
        fadeInTop();
    });
    $("#campus").mouseleave(function(){
        fadeInToronto();
        fadeInWaterloo();
    });


    $("#log-in-button").click(function(){sendLogin(); return false;});
    $("#forgotLogin").click(function(){location.href='signUp.html';});
    $("#signUp").click(function(){location.href='signUp.html';});

});


//-------------------------------------Toronto Animation Functions--------------------------
function fadeInToronto(){
    for (var i=1;i<=5; i++){
        $("#toronto"+i).fadeIn(300+i*200);
    }
}
function fadeOutToronto(){
    for (var i=1; i<=5; i++){
        $("#toronto"+i).fadeOut(1100-i*200);
    }
}

function fadeInTorontoStop(){
    for (var i=1;i<=5; i++){
        $("#toronto"+i).fadeIn().stop();
    }
}
function fadeOutTorontoStop(){
    for (var i=1; i<=5; i++){
        $("#toronto"+i).fadeOut().stop();
    }
}
//-----------------------------------End of Toronto Animation---------------------------


//-----------------------------------Waterloo Animation Functions----------------------------
function fadeInWaterloo(){
    $("#waterloo1").fadeIn(0);
    $("#waterloo2").fadeIn(300);
    $("#waterloo3").fadeIn(1300);
    $("#waterloo4").fadeIn(1600);
    $("#waterloo5").fadeIn(1800);
    $("#waterloo6").fadeIn(2000);
    $("#waterloo7").fadeIn(2000);
}

function fadeInWaterlooStop(){
    for (var i=1;i<=7; i++){
        $("#waterloo"+i).fadeIn().stop();
    }
}

function fadeOutWaterloo(){
    var num = 1100;
    for (var i=1; i<=7; i++){
        $("#waterloo"+i).fadeOut(num);
        num = num-200;
    }
}

function fadeOutWaterlooStop(){
    for (var i=1;i<=7; i++){
        $("#waterloo"+i).fadeOut().stop();
    }
}
//----------------------------------------End of Waterloo Animation-------------------


//-------------------------------------Campus Animation Functions--------------------
function fadeInTop(){
    for (var i=1;i<=4; i++){
        var id = "#top"+i;
        $(id).fadeIn(300+i*200);
    }
    $("#campus").fadeIn(1100);
}

function fadeOutTop(){
    $("#top1").fadeOut(1100);
    $("#top2").fadeOut(1100);
    $("#top3").fadeOut(900);
    $("#top4").fadeOut(700);
    $("#campus").fadeOut(500);
}

function fadeInTopStop(){
    for (var i=1;i<=4; i++){
        var id = "#top"+i;
        $(id).fadeIn().stop();
    }
    $("#campus").fadeIn().stop();
}

function fadeOutTopStop(){
    $("#top1").fadeOut().stop();
    $("#top2").fadeOut().stop();
    $("#top3").fadeOut().stop();
    $("#top4").fadeOut().stop();
    $("#campus").fadeOut().stop();
}
//-------------------------------------End of Campus Animation---------------
\$\endgroup\$
  • \$\begingroup\$ The first step to improve your code is to get rid of id's and use common classes. \$\endgroup\$ – elclanrs Sep 21 '13 at 1:15
2
\$\begingroup\$

The general rule is if you're doing something with a static element inside a event, you should cache that reference to this element.

$("#button").click(function(){
    $("#diagonalLine").hide(1500);
}

or

document.getElementById('button').onclick = function(){
    document.getElementById('diagonalLine').style.display='none';
}

Everytime the user click on #button it traverses the DOM to getElementById and find #diagonalLine. With jQuery there's much more overhead involved, it create a new jQuery object, parses the string #diagonalLine, find the element, bind it to the new object and returns it. There are all sort of thing jQuery do under hood to make it "magic".

var diagonalLine = $("#diagonalLine");
$("#centerButton").click(function(){
    diagonalLine.hide(1500);
}

var diagonalLine = document.getElementById('diagonalLine');
document.getElementById('centerButton').onclick = function(){
    diagonalLine.style.display='none';
}

You can also use closures for caching references to elements

$('#button').click=(function(){
    var diagonalLine = $("#diagonalLine");
    return function(){
        diagonalLine.hide(1500);
    }
})();


document.querySelector('#button').onclick=(function(){
    var diagonalLine = document.querySelector('#diagonalLine');
    return function(){
        diagonalLine.style.display='none';
    }
})();
\$\endgroup\$
  • \$\begingroup\$ To be pedantic, getElementById is usually implemented using a hash table rather than traversing the DOM, which makes it fairly quick. That said, constructing the jQuery object may make it much more expensive as you point out. \$\endgroup\$ – icktoofay Sep 21 '13 at 2:29
  • \$\begingroup\$ It may be good practice to name variables with dollar prefix to get a hint it's jQuery object: var $diagonalLine = $("#diagonalLine"); \$\endgroup\$ – Roman Susi Sep 21 '13 at 6:49
  • \$\begingroup\$ I think it comes to personal preference, IMO it adds pollution to the code. \$\endgroup\$ – Vitim.us Sep 21 '13 at 8:42
2
\$\begingroup\$

You could certainly use classes in many places to decrease code duplication. For instance, you could add a top class to all of your "top" elements. In that way, you could stop and fadeOut all in one line:

$(".top").fadeOut().stop();

Similarly, this:

for (var i=1;i<=4; i++){
    var id = "#top"+i;
    $(id).fadeIn().stop();
}

would be just this:

$(".top").fadeIn().stop();
\$\endgroup\$

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.