7
\$\begingroup\$

The following works as intended, however is there a more concise way to write this script without changing it too much? It's a simple menu with sub-levels which have a plus/minus icon when each li opens/closes.

jQuery:

$(document).ready(function() {
    $li = $('#main > li.expand');
    $li.click(function(){
        $ul = $(this).find('ul.sub-level').delay(200).slideDown(400);
        $(this).removeClass('expand').addClass('minus');
        $('#main > li > ul').not($ul).slideUp('slow');
        $('#main > li.minus').not($(this)).removeClass('minus').addClass('expand');

    })

    $li2 = $('.sub-level > li.expand');
    $li2.click(function(){
        $(this).find('ul.sub-level2').delay(200).slideToggle(400);
        $li2.toggleClass('minus').toggleClass('expand');
    })      
});

HTML:

<ul id="main">
<li class="expand">test-1
<ul class="sub-level">
    <li class="expand">sub-test-1
        <ul class="sub-level2">
            <li>sfasfasf</li>
            <li>sfafasf</li>
        </ul>
    </li>
  </ul>
  </li>
  <li>test-2</li>
  <li class="expand">test-3
  <ul class="sub-level">
    <li>sub-test-2</li>
    </ul>
   </li>
   <li>test-4</li>
   </ul>
\$\endgroup\$
1
  • 1
    \$\begingroup\$ One suggestion, add var before your variables. \$\endgroup\$
    – gen_Eric
    Jul 9, 2012 at 16:46

2 Answers 2

3
\$\begingroup\$

You want to make sure you always declare your variables before you assign. The reason for this is JavaScript will declare and assign in one statement but always on the global scope. This might overwrite important variables and cause you hours of debugging. The following has been cleaned up with some explanations.

// I personally prefer passing my ready callbacks directly to the jQuery function.
$(function() {
    // Declare a few constants for easier maintenance
    var delayTime = 200, slideDownTime = 400, slideUpSpeed = 'slow';

    // No need to declare or assign a variable here.
    $('#main > li.expand').click(function() { 
        // Lets declare this since we use it quite often
        var $this = $(this);
        // Modify the top element first 
        $this.removeClass('expand').addClass('minus');
        // No need to assign this result
        $this.find('ul.sub-level').delay(delayTime).slideDown(slideDownTime); 
        // Take advantage of the not selector
        $('#main > li > ul:not(.sub-level)').slideUp(slideUpSpeed);
        // Pass our declared variable instead and take advantage of toggleClass
        $('#main > li.minus').not($this).toggleClass('minus expand');
    });

    // Declare and assign. Maybe come up with a better name?
    var $li = $('.sub-level > li.expand');
    $li.click(function(){
        $(this).find('ul.sub-level2').delay(delayTime).slideToggle(slideDownTime);
        $li.toggleClass('minus expand');
    });      
});
\$\endgroup\$
3
  • 1
    \$\begingroup\$ $li.toggleClass('minus').toggleClass('expand'); could more simply be: $(this).toggleClass('minus expand'). \$\endgroup\$
    – gen_Eric
    Jul 9, 2012 at 18:10
  • \$\begingroup\$ @Rocket - Nice catch, I didn't even notice it when I reviewed your answer. \$\endgroup\$ Jul 9, 2012 at 18:37
  • \$\begingroup\$ In my answer, I abstracted that out to its own function. \$\endgroup\$
    – gen_Eric
    Jul 9, 2012 at 18:39
6
\$\begingroup\$

Some suggestions:

  • Use var when declaring variables, unless you really want to make global variables (which you probably don't)
  • Combine var statements using ,

    var a = 1, b = 2, c = 3;
    
  • Combine classes in toggleClass

    $(this).toggleClass('minus expand')
    
  • Cache selectors that are used often (like $('#main'))
  • You don't need to use $li2 inside the click handler, you can just use $(this)
  • Turn repeated code into a function
$(function(){
    var $main = ('#main'),
    slide = function(ele, find){
        return $(ele).toggleClass('minus expand').find(find).delay(200).slideToggle(400);
    },
    $li = $('> li.expand', $main).click(function(){
        var $ul = slide(this, 'ul.sub-level');
        $('> li > ul', $main).not($ul).slideUp('slow');
        $('> li.minus', $main).not(this).toggleClass('minus expand')
    }),
    $li2 = $('.sub-level > li.expand').click(function(){
       slide(this, 'ul.sub-level2');
    });
});
\$\endgroup\$
8
  • \$\begingroup\$ You don't appear to need $li or $li2 at all. It might be useful though to extract and name some of the constants in there in case you need to discover/change them in the future. \$\endgroup\$
    – Bill Barry
    Jul 9, 2012 at 18:06
  • \$\begingroup\$ @BillBarry: I kept $li and $li2, because I figured this script was part of a bigger program, and that maybe they were used again elsewhere. \$\endgroup\$
    – gen_Eric
    Jul 9, 2012 at 18:09
  • 1
    \$\begingroup\$ @Rocket - After reviewing your answer again I like the way you refactored the slide logic. \$\endgroup\$ Jul 9, 2012 at 18:41
  • \$\begingroup\$ @ChaosPandion: Thanks. I prefer using functions instead of writing the same thing multiple times. \$\endgroup\$
    – gen_Eric
    Jul 9, 2012 at 18:42
  • \$\begingroup\$ You don't need to use $li2 inside the click handler, you can just use $(this) - Why create a new jQuery object every time the element is clicked? \$\endgroup\$ Jul 10, 2012 at 22:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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