How to improve this jQuery code for tabs

Question

I'm quite new to javaScript and jQuery and wrote the following function to creat some dynamic tabs. I'd be very interested to know how professional programmers would improve this code.

jsFiddle

JavaScript

$(document).ready(function(){

function createVerticalTabs(options){   

    var container = options["container"];
    var parentElement = options["parentElement"];
    var childElement = options["childElement"];
    var firstElement = options["firstElement"];
    var tabClass = options["tabClass"];
    var tabClassActive = options["tabClassActive"];
    var closeOtherElements = options["closeOtherElements"]
    var preventEvent = options["preventEvent"]; 

    if(firstElement){
        $(container+" "+childElement).not(":first").hide();
        classToggle($(container+" "+childElement+":first").prev(parentElement), tabClass, tabClassActive);
    }else{
        $(container+" "+childElement).hide();
    }

    /* Click event */
    $(container+" > "+parentElement).on("click", function(event){
        if(preventEvent == true){
            event.preventDefault();
        }

        /* Check if there're other active elements and close them if requested*/

        if(closeOtherElements){
            var otherActiveElement = $(this).parent().children(childElement);

            if($(otherActiveElement).is(":visible")){
                $(otherActiveElement).slideUp("slow");
                classToggle($(otherActiveElement).prev(parentElement), tabClassActive, tabClass);
            }
        }

        /* Show requested element */
        var details = $(this).next(childElement);

        if($(details).is(":visible")){
            classToggle($(this), tabClassActive, tabClass);
            $(details).slideUp("slow");
        }else{
            classToggle($(this), tabClass, tabClassActive);
            $(details).slideDown("slow");
        }

    });


    function classToggle(obj, remove, add){
        return obj.removeClass(remove).addClass(add);
    }


}

var options = {
    container: ".testBox",
    parentElement: "a",
    childElement: "p",
    tabClass: "title",
    tabClassActive: "active",
    firstElement: true,
    closeOtherElements: true,
    preventEvent: true
};

var options2 = {
    container: ".testBox2",
    parentElement: "h3",
    childElement: "p",
    tabClass: "title",
    tabClassActive: "active",
    firstElement: false,
    closeOtherElements: false,
    preventEvent: true
};

createVerticalTabs(options);
createVerticalTabs(options2);
});

HTML

<div class="testBox">
    <a href="#">Anchor 1</a><p>Details of Anchor 1</p>
    <a href="#">Anchor 2</a><p>Details of Anchor 2</p>
    <a href="#">Anchor 3</a><p>Details of Anchor 3</p>
    <a href="#">Anchor 4</a><p>Details of Anchor 4</p>
</div>
<div class="testBox2">
    <h3>Anchor 1</h3><p>Details of Anchor 1</p>
    <h3>Anchor 2</h3><p>Details of Anchor 2</p>
    <h3>Anchor 3</h3><p>Details of Anchor 3</p>
    <h3>Anchor 4</h3><p>Details of Anchor 4</p>
</div>
<div style="clear:both"></div>

CSS

.testBox, .testBox2{ float:left; height:500px; margin:10px; }
.testBox a{ display:block; }

.testBox2 h3{ margin:0; padding:0; }

.title{ font-weight:bold; }
.active{ background:#333; border:#000 1px solid; color:#fff; }

Answer 1

I'm going to go through a few main points here about your code but don't be overwhelmed by the size of this book, I prefer to be on the side of too much information than too little.

- Cache your selectors: Probably the most important thing you can do for your code now. 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 queries the DOM to try and find elements that match. So imagine that every time you do that it runs a search. Would it be better if you could save the search results? This way you can look and play with them whenever you want without having to look for them again. Ex.:

//Something like this:

var childFound = $(container).find(childElement); 
/* Here I saved my search to a variable, which I use later.
This will return all "p" elements in "container".
You can filter more and get first element or all the others
The .find() method is just an example, there are countless others you can use
to help you select the elements you want
Methods like .map(), .each(), and .nextAll() will be increadibly useful for you */

//Then use like this
childFound.hide();

You're on the right track by setting up the options variable. This concept will be vital when you start to develop plugins, if that's what you want to do.

- IIFE's: If you have your code in the footer (which you should be doing) what you'll want to do is wrap it in a IIFE. "What the hell is that?" you may ask. A basic syntax looks like this:

(function () {
    //Code goes in here.
})();

You'll see several variations of this all over the internet as you read to find out more. The one I recommend you use in this case is like this:

;(function ($, window, document) {
    //Code goes here and the $ belongs to jQuery
})(jQuery, window, document);

So lets break this down. The ; in the front is a safety net against unclosed scripts, which can be common if you use plugins in Wordpress, or if you concat and minify your files it will protect you from your function becoming an argument.

Then we pass in $, window, and document. We pass in $ and at the bottom assign it to jQuery. So now in this function, no matter what value the $ carried outside, in here it's jQuery. Then we pass in window and document. These are optional in your case, but a good idea since you do use references to window in your code. It saves window as a local variable, and also will be good when you minify your code as the window reference can be changed to something like a or foo automatically. If you don't use a window or document reference you can just remove them all together.

Now keep in mind there might be times where you have to put your script in the header. Modernizr is an example of this. Then you'll probably end up using the document.ready. Don't sweat because all you have to do is this:

jQuery(document).ready(function( $ ) {
    //Code goes here and $ will belong to jQuery.
});

I wouldn't rely on $.noConflict to protect your code from other libraries that use the $ as well.

- Click Event Handler: As was mentioned before, there are several ways to set up your events. The ones that were mentioned were .click() and .on("click"). The .click() method simply calls the .on() method and passes in the click. The .on() method is incredibly useful since you can use it to set up almost any kind of event - not only clicks. So yes, the one you are using now is "the best" because it saves you a function call. Now saving a single function call in your app won't be a significant increase in performance and you probably won't even notice it. Although arguing over such a small and possibly insignificant changes is really what we developers do best.

Using the .on() method directly is hands down going to be faster, but there are many other things you could be spending your time on that will generate more significant performance results.

- Don't re-invent the wheel: I strongly believe that if there's a working solution out there by all means use it. There are tons of tabs and accordion plugins available I'm sure you can find one that fits your project. If you can't find an exact fit, you can still always tweak it and make it fit. jQuery it self has an entire UI package with tabs, accordions, sliders, and all kinds of cool stuff just ready and waiting to be used. Also the jQuery UI will let you use your own CSS files and customizations really easily. Making a whole new app from scratch should be done if absolutely necessary, or if you're trying to learn (which is your case). Back to the subject of learning, like you said, you are just starting out with jQuery, and with that I highly recommend this screencast by Jeffrey Way called 30 Days to Learn jQuery. He does a really good job of explain some basic principals as well as some more complex concepts.

One thing in specific I'm going to point out is the following:

var details = $(this).next(childElement); //Details here is a jQuery object 
//You don't need to wrap it again, just use details.someMethod();
if(details.is(":visible")){
    classToggle($(this), tabClassActive, tabClass);
    details.slideUp("slow");
}else{
    classToggle($(this), tabClass, tabClassActive);
    details.slideDown("slow");
}

When you cache a selection like you did there, the .next() gives you a new jQuery object with the results from the search which then are stored in details. So doing this: $(details) is the same as doing $($("p")). You're wrapping it twice. This is a common mistake with beginners and Jeffery, in that screencast, explains it much better than I ever could.

Answer 2

• I wouldn't change the entire code, but for performance you could compress the code. Closure compiler
  
• For a better UI I prefer twitters bootstrap (a sliding plugin included). Bootstrap
• Use .click instead of .on('click')
• Save variables you use often in a variable. For example $(otherActiveElement) or $(this)