3
\$\begingroup\$

I have a JavaScript/jQuery function that handles the drop-down menus for several different sites. I am a new to JavaScript and jQuery and would love any input on how to clean up my code. Essentially, I am asking how to make this script more efficient and less clunky, so I can improve performance and write better code.

   <!doctype html>
<html>

  <head>

  <title> </title>

  <!--stylesheets-->
 <link rel="stylesheet" href="../css/reset.css"> 
<link rel="stylesheet" href="../css/master.css">

 <!--custom fonts-->



  <!--JS-->
  <script src="http://ajax.googleapis.com/ajax/libs/jquery/1.6.4/jquery.min.js">         </script>
  <script src="functions.js"></script>

   <script>

 $(function() {

    dropdownSlide('.ppt-menu', 'ul.sub-menu', 5000, fade)

})



    </script>

</head>

<body>
<nav id="test-menu" class="ppt-menu">
    <ul id="menu-main-nav-1" class="menu">

<li class="menu-item menu-item-type-post_type menu-item-object-page menu-item-63">      <a href="http://blakecommunications.com/">Home</a></li>
      <li class="menu-item menu-item-type-custom menu-item-object-custom menu-item-65"><a href="http://blakecommunications.com/about">About</a> 

        <ul class="sub-menu">

           <li class="menu-item menu-item-type-post_type menu-item-object-page menu-item-60"><a href="http://blakecommunications.com/about/approach/">Approach</a></li>
        <li class="menu-item menu-item-type-post_type menu-item-object-page menu-item-62"><a href="http://blakecommunications.com/about/industries/">Industries</a></li>
        <li class="menu-item menu-item-type-post_type menu-item-object-page menu-item-91"><a href="http://blakecommunications.com/about/services/">Services</a></li>
        <li class="menu-item menu-item-type-post_type menu-item-object-page menu-item-61"><a href="http://blakecommunications.com/about/stephanie-blake-bio/">Bio</a></li>

    </ul>

</li>

<li class="menu-item menu-item-type-custom menu-item-object-custom menu-item-120"><a href="http://blakecommunications.com/projects">Projects</a> 

    <ul class="sub-menu">

        <li class="menu-item menu-item-type-post_type menu-item-object-page menu-item-93"><a href="http://blakecommunications.com/projects/uccs/">UCCS</a></li>
        <li class="menu-item menu-item-type-post_type menu-item-object-page menu-item-92"><a href="http://blakecommunications.com/projects/the-dixon-collective/">The Dixon Collective</a></li>

    </ul>

</li>

<li class="menu-item menu-item-type-custom menu-item-object-custom menu-item-120"><a href="http://blakecommunications.com/projects">Projectsttow</a> 

    <ul class="sub-menu">

        <li class="menu-item menu-item-type-post_type menu-item-object-page menu-item-93"><a href="http://blakecommunications.com/projects/uccs/">UCCS</a></li>
        <li class="menu-item menu-item-type-post_type menu-item-object-page menu-item-92"><a href="http://blakecommunications.com/projects/the-dixon-collective/">The Dixon Collective</a></li>

    </ul>

</li>

<li class="menu-item menu-item-type-post_type menu-item-object-page menu-item-57"><a href="http://blakecommunications.com/contact/">Contact</a></li>

function dropdownMenu(nav, dropdown, speed, type) {

var nav = $(nav), dropdown = $(dropdown);

var parentItem = $(nav).find('li').has(dropdown);

$(parentItem).find(dropdown).hide();

if( type === slide ) {

    $(parentItem).mouseenter(function () {
        $('li').find(dropdown).hide();
        $(this).find(dropdown).stop(true, true).slideDown(speed);
    }).mouseleave(function () {
        $(this).find(dropdown).delay(500).slideUp(speed);
    });

}

else if ( type === fade ) {

    $(parentItem).mouseenter(function () {
        $('li').find(dropdown).hide();
        $(this).find(dropdown).stop(true, true).fadeIn(speed);
    }).mouseleave(function () {
        $(this).find(dropdown).delay(500).fadeOut(speed);
    });

}

else if( type === show) {

    $(parentItem).mouseenter(function () {
        $('li').find(dropdown).hide();
        $(this).find(dropdown).stop(true, true).show(speed);
    }).mouseleave(function () {
        $(this).find(dropdown).delay(500).hide(speed);
    });

}

else {
    $(parentItem).mouseenter(function () {
        $('li').find(dropdown).hide();
        $(this).find(dropdown).stop(true, true).show();
    }).mouseleave(function () {
        $(this).find(dropdown).delay(500).hide();
    });
}

 }
\$\endgroup\$
  • \$\begingroup\$ Please show us the corresponding HTML to we can see how to use it's structure to help make the code smarter. \$\endgroup\$ – jfriend00 Nov 16 '11 at 1:53
  • \$\begingroup\$ When you pass dropdown into your function, what is it? selector? DOM element? It looks like there's probably some wastage around that, but I don't know exactly what it is. Please add a sample call to the dropdownMenu function that passes in things used in your HTML markup. \$\endgroup\$ – jfriend00 Nov 16 '11 at 4:48
3
\$\begingroup\$

You've made a few mistakes.

Firstly you used $(parentItem) and $(nav) when you don't need to, because they are already jQuery objects.

Secondly you copy pasted 4 blocks of similar codes without abstracting it into a function.

Then the function itself could have been optimised, by removing the $("li").find(dropdown) call outside the event handlers.

function dropdownMenu(nav, dropdown, speed, type) {

    var nav = $(nav),
        dropdown = $(dropdown);

    var parentItem = nav.find('li').has(dropdown);

    parentItem.find(dropdown).hide();

    function action (enter, leave, speed) {
        var all = $('li').find(dropdown);

        parentItem.mouseenter(function() {
            all.hide();
            $(this).find(dropdown).stop(true, true)[enter](speed);
        }).mouseleave(function() {
            $(this).find(dropdown).delay(500)[leave](speed);
        }); 
    }

    switch (type) {
        case "slide":
            action ("slideDown", "slideUp", speed); break;
        case "fade":
            action ("fadeIn", "fadeOut", speed); break;
        case "show":
            action ("show", "hide", speed); break;
        default: 
            action ("show", "hide"); break;
    }
}

The rest of the optimisations depend on the HTML markup.

\$\endgroup\$
  • \$\begingroup\$ Thanks. Question: Should the if/else statement be checking for a string? Or should I pass type === "slide" or just slide. If I don't set it to a string I get a console error 'slide is not defined'. \$\endgroup\$ – Jacobi Nov 16 '11 at 3:42
  • \$\begingroup\$ @Jacobi In that case it should be a string. so "slide" and you should pass in a string as an argument \$\endgroup\$ – Raynos Nov 16 '11 at 3:51
2
\$\begingroup\$

Without seeing the markup, I'd suggest this table-driven approach which is easy to extend to more types by just adding another item to the table. Based on your comments in other posts, I'm also assuming that the type is actually a string with values: "slide", "fade", "show", ...

function dropdownMenu(nav, dropdown, speed, type) {

    var dropdown = $(dropdown);
    var parentItem = $(nav).find('li').has(dropdown);
    parentItem.find(dropdown).hide();

    var options = {
        slide: {in: "slideDown", out: "slideUp", speed: speed},
        fade: {in: "fadeIn", out: "fadeOut", speed: speed},
        show: {in: "show", out: "hide", speed: speed},
        other: {in: "show", out: "hide"}
    };

    var data = options[type] || options.other;

    parentItem.mouseenter(function () {
        $('li').find(dropdown).hide();
        $(this).find(dropdown).stop(true, true)[data.in]](data.speed);
    }).mouseleave(function () {
        $(this).find(dropdown).delay(500)[data.out](data.speed);
    });
}
\$\endgroup\$
  • \$\begingroup\$ There are a few micro optimisations you can do. $(nav) on line 4 is redudant. $(parentItem) on line 5 is redundant. Creating options as a local variable is redundant (bind it in closure scope to the function declaration). The if (!data) can be made more elegant using ||. And the $("li")... in the mouseenter handler is being created every time for no reason. \$\endgroup\$ – Raynos Nov 16 '11 at 3:59
  • \$\begingroup\$ I didn't attempt to figured out how the HTML works without seeing it (it wasn't available at the time I wrote this). I just attacked the duplication of the event handlers. \$\endgroup\$ – jfriend00 Nov 16 '11 at 4:01
  • \$\begingroup\$ Updated the parentItem reference and removed the if (!data). \$\endgroup\$ – jfriend00 Nov 16 '11 at 4:15
  • \$\begingroup\$ Removed nav local variable as it doesn't seem to be needed. \$\endgroup\$ – jfriend00 Nov 16 '11 at 5:01

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.