Modifying a menu

Question

I have a hard time adapting to jQuery and chaining. I think it's really neat at times, but how do I know when to use it and when to indent? I may suffer from some kind of OCD-ish behaviour, and this is not your concern, but I would like your input on what needs refactoring in this ol' bit of code:

$(function() {
    $('div.mainMenu > ul > li > a')
        .each(
            function () {
                var $this = $(this);
                var $sub = $this.next('ul');
                var n = $sub.children().length;
                $sub.hide();

                if (n > 1) {
                    $this.append('<img src="img/expand.png">');
                } else if (n == 1) {
                    $this.attr('href', $sub.find('a').first().attr('href'));
                }
            }
        )
        .click(
            function (event) {
                var $this = $(this);
                var $sub = $this.next('ul');
                var n = $sub.children().length;

                if (n > 1) {
                    event.preventDefault();
                    var $img = $this.find('img');

                    if ($sub.is(':visible')) {
                        $img.attr('src', 'img/expand.png');
                    } else {
                        $img.attr('src', 'img/collapse.png');
                    }

                    $sub.slideToggle(n * 100);
                }
            }
        )
        .parent()
            .find('li > ul')
                .prev('a')
                    .append('<img src="img/submenu.png">');
});

I'm essentially modifying a menu and everything works, but there are some inconsistencies. Sometimes I use the "newline and indent" approach and other times it's the good ol' Java chaining that I'm used to. Is this good jQuery code, or how would you rewrite it?

Also, is there a way to incorporate chaining instead of my very simple conditionals (if (n > 1)...)? If there are any other bad smells in this code, please let me know. I feel so insecure when diving into something new.

Answer 1

Your code looks pretty decent IMHO. Here is how I prefer to do it:

(function($) { // Note that $ is now a parameter of the function and jQuery is added at the end - this is known as a "wrapping method" and avoids any potential conflicts with other libraries that may use the dollar sign
   "use strict"; // Put the browser's parser into strict mode if supported. Warning: may make you a better programmer

    $('div.mainMenu > ul > li > a').each(function() {
        var $this = $(this),
            $sub = $this.next('ul'),
            n = $sub.children().length; // All variables are grouped together in one var statement

        $sub.hide();

        if (n > 1) {
            $this.append('<img src="img/expand.png">');
        } else if (n == 1) {
            $this.attr('href', $sub.find('a').first().attr('href'));
        }
    }).on("click", function(event) { // "click" here has been changed to on("click", ....) to match the new unified syntax in 1.7+
        var $this = $(this),
            $sub = $this.next('ul'),
            n = $sub.children().length;

        if (n > 1) {
           event.preventDefault();

           var $img = $this.find('img');

           if ($sub.is(':visible')) {
               $img.attr('src', 'img/expand.png');
           } else {
               $img.attr('src', 'img/collapse.png');
           }

           $sub.slideToggle(n * 100);
        }
    }).parent().find('li > ul').prev('a').append('<img src="img/submenu.png" />');
})(jQuery); // Pass jQuery to the wrapping function as a parameter

Note: the code in your two functions is similar, so consider moving it to a separate function to keep it DRY. Additionally, checking your code with jsLint/jQueryLint will help you track down code issues (although it might make you cry).