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>

• gen_Eric: One suggestion, add var before your variables.

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

• gen_Eric: $li.toggleClass('minus').toggleClass('expand'); could more simply be: $(this).toggleClass('minus expand').
• ChaosPandion: @Rocket - Nice catch, I didn't even notice it when I reviewed your answer.
• gen_Eric: In my answer, I abstracted that out to its own function.

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

• Bill Barry: 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.
• gen_Eric: @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.
• ChaosPandion: @Rocket - After reviewing your answer again I like the way you refactored the slide logic.
• gen_Eric: @ChaosPandion: Thanks. I prefer using functions instead of writing the same thing multiple times.
• Joseph Silber: 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?