1
\$\begingroup\$

I created my first jQuery plugin over this weekend. I would like a code review so that I can improve my code in the future and learn from any mistakes I make. Any tips for Github would also be greatly appreciated since I am also new to GitHub :).

Demo: https://rawgithub.com/Shivambh28/ezToggle/master/demo.html

GitHub: https://github.com/Shivambh28/ezToggle

Plugin code:

(function($){
    $.fn.ezToggle = function(options) {
        var defaults = {
            selector: '.selector',
            speed : 200
        }
        defaults.minHeight = $(defaults.selector).outerHeight(true);
        var options =  $.extend(defaults, options);
        return this.each(function() {
            var o    = options,
                yDiv = $(this),
                oHei = $(yDiv).height();
            $(yDiv).not('.opened').addClass('closed');
            $('.closed').height(defaults.minHeight);
            $('.opened').height(oHei);
            $(defaults.selector).click(function() {
                if ($(this).closest(yDiv).hasClass('closed') ) {
                    $('.opened').removeClass('opened').addClass('closed').animate({height:defaults.minHeight}, defaults.speed);
                    $(this).closest(yDiv).removeClass('closed').addClass('opened').animate({height:oHei}, defaults.speed);
                } else if ( $('.opened').length == 1 ) {
                    $(this).closest(yDiv).removeClass('opened').addClass('closed').animate({height:defaults.minHeight}, defaults.speed);
                }
            });
        });
    };
})(jQuery);
\$\endgroup\$
  • \$\begingroup\$ "Uncaught ReferenceError: jQuery is not defined " <- on your Demopage. Reason: "[blocked] The page at rawgithub.com/Shivambh28/ezToggle/master/demo.html ran insecure content from code.jquery.com/jquery-1.10.1.min.js." \$\endgroup\$ – Thomas Junk Jun 24 '13 at 17:12
  • \$\begingroup\$ Thanks, I fixed the issue. I notice a strange issue related to the border-radius on chrome. It appears as if when I click on a div, the border-radius-top turns to 0. I wonder why this might be occurring.. \$\endgroup\$ – Shivam Bhalla Jun 24 '13 at 18:42
  • \$\begingroup\$ Nevermind, fixed border-radius issue as-well. \$\endgroup\$ – Shivam Bhalla Jun 24 '13 at 19:04
3
\$\begingroup\$

Props to you on your first plugin! Well here are a few pointers in no particular order:

- Double Wrapping: When you cache yDiv=$(this);, you're wrapping the element with the jQuery object. And then you do: $(yDiv).not(... What this does is wrap that object twice. You've already wrapped it so you don't need to do so again. Just do this yDiv.not(... after you cache.

So this is how you should do it:

//Don't
yDiv = $(this);
$(yDiv).not('.opened').addClass('closed');

//Instead do:
yDiv = $(this);
yDiv.not('.opened').addClass('closed');

- Variable Names: If you want anyone else to provide you input on your code, they have to be able to understand it. You should use variable names that will make sense to anyone reading your code. Yes, in this simple example one can deduce the what and why you named the variables the way you did and we can sort of follow along. But when you start working on large scale projects, things can get out of hand real quick. Take for example your yDiv variable. The keyword this can refer to any element that goes through the .each() which might not be a div in the first place. What if it was a ul or any other element. In that case, the name yDiv would no longer represent what is actually stored in there. Anyways, try to use meaningful and descriptive variable names and stay away from names that might not exactly represent what is stored in said variable.

- Cache your selectors: As a rule of thumb, if you use a selection more than once, you should cache it. What happens when you use $("someElem") is jQuery has to jump into the DOM and look through all elements that would match that selection. So you should really do a search only once, and save your results for future use. This way you can look and play with them whenever you want without having to try and find them again.

Ex:

var closestElem = $(this).closest(yDiv); //Cache it
if (closestElem.hasClass('closed')){ //Now use it
    //...
}

- Save a function calls: If you read the jQuery source code you'll see that shortcut methods like .click(), .scroll(), and etc. all reference the .on() method. All they do is basically call the .on() method with some parameters. Well why not just go straight to the meat and potatoes?

Check this out:

$("#foo").click(function() {
    //Do your awesomeness
});

//That does the same thing as this:
$("foo").on("click", function() {
    //Do your awesomeness even awesomener.
});

//You can also use that to set up your own custom events
$("foo").on("myEvent", function() {
    //bar
});

That just makes the syntax so easy to remember as well as provide you with enormous control over your events. If you're going to use jQuery on a regular basis, I recommend that you take some time and read through the source code of the methods you're using. To find them quickly just use CTRL+F and type methodName:. That should jump you straight to what you want to know. This way you can understand what and how they are doing things, and even find better ways to do them on your own. Also if you see something you think should be done better or differently, you understand how it works and you can contribute to jQuery.

\$\endgroup\$
  • \$\begingroup\$ Thanks so much for the in-depth review of the code and tips, I really took everything you told me into consideration and redid the entire plugin from scratch. It is much more flexible now and more clean and easier to read. Thanks again for your help! \$\endgroup\$ – Shivam Bhalla Jul 4 '13 at 1:24
  • \$\begingroup\$ Open another question with your updated code and link back to this one so we can see your improvements. Other people might have more to add, and you might still have more to learn. \$\endgroup\$ – Jonny Sooter Jul 5 '13 at 15:06
  • \$\begingroup\$ Sure, I just re-posted the question: codereview.stackexchange.com/questions/28195/… \$\endgroup\$ – Shivam Bhalla Jul 6 '13 at 13:55
1
\$\begingroup\$

+1 for Jonny Sooter.

I just linted your code plus followed Jonny Sooter's advice. I didn't renamed any variables as such, but made huge changes to formatting and readability. Gurus feel free to edit this code if there is any stupidity.

(function($){
    'use strict';
    $.fn.ezToggle = function(opts) {
        var 
            defaults = {
                selector: '.selector',
                speed : 200
            },
            options
        ;

        defaults.minHeight = $( defaults.selector ).outerHeight( true );
        options = $.extend( defaults, opts );

        return this.each( function() {
            var 
                yDiv = $(this),
                oHei = yDiv.height();

            yDiv
                .not( '.opened' )
                .addClass( 'closed' );

            $( '.closed' ).height( defaults.minHeight );
            $( '.opened' ).height( oHei );

            $( defaults.selector ).on( 'click', function() {
                if ( $(this).closest( yDiv ).hasClass( 'closed' ) ) {
                    $('.opened')
                        .removeClass('opened')
                        .addClass('closed')
                        .animate({
                                height: defaults.minHeight
                            }, 
                            defaults.speed
                        );
                    $(this)
                        .closest(yDiv)
                        .removeClass('closed')
                        .addClass('opened')
                        .animate({
                                height: oHei
                            }, 
                            defaults.speed
                        );
                } else if ( $('.opened').length === 1 ) {
                    $(this)
                        .closest(yDiv)
                        .removeClass('opened')
                        .addClass('closed')
                        .animate({
                                height: defaults.minHeight
                            }, 
                            defaults.speed
                        );
                }
            });
        });
    };
}(jQuery));
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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