Clean and efficient JavaScript/jQuery?

Question

I have a JavaScript/jQuery function that handles the drop down menu's 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.

Thanks.

html:

   <!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();
    });
}

 }

Please show us the corresponding HTML to we can see how to use it's structure to help make the code smarter.
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.

Raynos · Answer 1 · 2011-11-16 03:53:19Z

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.

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'.
@Jacobi In that case it should be a string. so "slide" and you should pass in a string as an argument

jfriend00 · Answer 2 · 2011-11-16 05:01:32Z

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);
    });
}

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.
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.
Updated the parentItem reference and removed the if (!data).
Removed nav local variable as it doesn't seem to be needed.

asked	1 year ago
viewed	376 times
active	1 year ago

Clean and efficient JavaScript/jQuery?

2 Answers

Your Answer

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

Community Bulletin

Clean and efficient JavaScript/jQuery?

2 Answers

Your Answer

Sign up or log in

Post as a guest

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

Community Bulletin

Related