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.

I'm trying to come up with a way to make a drop down box that is displayed through a jquery mouse hover event and with nested dropdown boxes displayed through hovering over elements of the original drop down box. I wrote some terribly inefficient code and I'm struggling to find ways of simplifying it. If anyone has any suggestions that will help me shorten this code and get a better idea of how to take advantage of functions of JQuery, please help.

here is the link:

http://cs-dev.dreamhosters.com/dropd.php

$(document).ready(function(){
$(".tab, .drop").hover(function(){
$(".tab").css("color","#FF7722");
$(".drop").css("display","block");
$("#tv, .droptv").hover(function(){
        $(this).css("color","#FF7722");
        $(".droptv").css("display","block");
        $(".droptv").hover(function(){
                $("#tv, .droptv").css("color","#FF7722");
                },function(){
                $(".droptv").css("color","#005BAB");
                });
        },function(){
        $(this).css("color","#005BAB");
        $(".droptv").css("display","none");
        });
$("#interact").hover(function(){
        $(this).css("color","#FF7722");
        },function(){
        $(this).css("color","#005BAB");
        });
$("#online").hover(function(){
        $(this).css("color","#FF7722");
        },function(){
        $(this).css("color","#005BAB");
        });
$("#vod, .dropvod").hover(function(){
        $(this).css("color","#FF7722");
        $(".dropvod").css("display","block");
        $(".dropvod").hover(function(){
                $("#dai").hover(function(){
                    $(this).css("color","#FF7722");
                    },function(){
                    $(this).css("color","#005BAB");
                    });
                $("#iguide").hover(function(){
                    $(this).css("color","#FF7722");
                    },function(){
                    $(this).css("color","#005BAB");
                    });
                $("#vod").css("color","#FF7722");
                },function(){
                $(".dropvod").css("color","#005BAB");
                });
        },function(){
        $(this).css("color","#005BAB");
        $(".dropvod").css("display","none");
        });
$("#tablet").hover(function(){
        $(this).css("color","#FF7722");
        },function(){
        $(this).css("color","#005BAB");
        });
$("#mobile").hover(function(){
        $(this).css("color","#FF7722");
        },function(){
        $(this).css("color","#005BAB");
        });
},function(){
$(".tab").css("color","#005BAB");
$(".drop").css("display","none");
 });
});
share|improve this question
    
Try using JavaScript for state and CSS for style. It would make this code so much simpler! –  ANeves Dec 6 '12 at 10:08

2 Answers 2

up vote 4 down vote accepted

Your code is very big and messy so this a bit tricky to really see what you are trying to do. A few obvious things to make the code more readable:

  • Replace .css("display","block") with .show()
  • Replace .css("display","none") with .hide()
  • You repeat the same color changing hover over and over again. Instead, group all your elements together and specify this function only once:

    $("#containo div").hover(function() {
    $(this).css("color","#FF7722");
    },function() {
    $(this).css("color","#005BAB");
    });

  • Do not nest hovers inside other hovers. .hover() creates a new event handler when it is called. If you nest them then you each time you move your mouse over the parent, the child is assigned a new event handler. You do not want these duplicates. Instead assign all of your events in the root level.

 

This, plus a little refactoring could reduce your code considerably. Maybe from here the code will be easier to work with so could see how to reduce it further.

$(document).ready(function() {

    function on(selector) {
        $(selector).css("color","#FF7722");
    };
    function off(selector) {
        $(selector).css("color","#005BAB");
    };

    $("#interact,#online,#tablet,#mobile,#dai,#iguide,#tv,.droptv,#vod,.dropvod")
    .hover(function(){
        on(this)
    },function(){
        off(this)
    });

    $("#tv, .droptv").hover(function(){
        $(".droptv").show();
    },function(){
        $(".droptv").hide();
    });

    $("#vod, .dropvod").hover(function(){
        on("#vod");
        $(".dropvod").show();
    },function(){
        off(".dropvod");
        $(".dropvod").hide();
    });

    $(".tab, .drop").hover(function(){
        on(".tab");
        $(".drop").show();
    },function(){
        off(".tab");
        $(".drop").hide();
    });
});
share|improve this answer
    
Thank you for the help, buh buh. –  Drew Paul Dec 6 '12 at 13:50

I would also suggest, in addition to Buh Buh's answer:

  • Save the references to the elements in a variable, so that instead of $('.tv') and making jQuery perform a search through the DOM every time, you can refer to your variable and apply jQuery methods to it. Furthermore, this can help if you also can give meaningful names to those variables. For example

    var $mainCombos = $('.tv'); // later on $mainCombos.show();

I always use a dollar sign to indicate which of my variables are jQuery objects versus regular variables, but it is really not needed.

Beware that this may bring you problems if you're dynamically adding or removing elements from the DOM. If this is the case, you may use alternative versions of this, like re-setting the reference variable each time your code is called, or applying live events and grasping the references with $(this).

  • Chain calls to jQuery methods whenever possible. This prevents jQuery from searching all over again for those elements (also useful if you can't apply my first suggestion). For instance:

Instead of

$('.droptv').show();
$('.droptv').hover(...);

You could use

$('.droptv').show()
            .hover(...);
share|improve this answer

Your Answer

 
discard

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

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