Simple menu with sub-levels

Question

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>

Answer 1

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

Answer 2

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