Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm using basic jQuery to render tab data. I was wondering if anybody could comment on how to refactor the click() event handlers, and any general guidance or advice.

Here's the code/output in jsFiddle

Please see snapshot below of the main lines of code I'd like to clean up.

It would also be great if you can point me to any resources that might be beneficial.

('#tab1').click(function() {
  $("#page").empty().html("<p>Here is our menu: Crispy Duck, Shrimp Tempura, Christmas Roll</p> ");
  $("a").removeClass("active");
  $(this).addClass("active");
});

$("#tab2").click(function() {
  $("#page").empty().html("<p>We're located at: 13352 Minnieville Rd., Woodbridge, VA </p>");
  $("a").removeClass("active");
  $(this).addClass("active");
});

$("#tab3").click(function() {
  $("#page").empty().html("<p>We can be reached at: (555)555-5555.</p>");
  $("a").removeClass("active");
  $(this).addClass("active");
});
share|improve this question
add comment

3 Answers 3

up vote 5 down vote accepted

You can combine these using a helper function with a closure.

function bindTabClickToSetPageContent(selector, content) {
    $(selector).click(function () {
        $("#page").html(content);
        $("a").removeClass("active");
        $(this).addClass("active");
    });
}

bindTabClickToSetPageContent("#tab1", 
    "<p>Here is our menu: Crispy Duck, Shrimp Tempura, Christmas Roll</p>");
bindTabClickToSetPageContent("#tab2", 
    "<p>We're located at: 13352 Minnieville Rd., Woodbridge, VA</p>");
bindTabClickToSetPageContent("#tab3", 
    "<p>We can be reached at: (555)555-5555.</p>");

Note: I dropped the call to empty since html performs the same function before replacing the content.

share|improve this answer
    
I like this approach. Also thanks for the note regarding the empty call. –  Jamie B Jun 25 at 22:37
add comment

You can simply put your common code in a function

function giveItAName(htmlValue,self){

 $("#page").empty().html(htmlValue);
 $("a").removeClass("active");
 $(self).addClass("active");
}

And call this function accordingly

('#tab1').click(function() {
giveItAName("<p>Here is our menu: Crispy Duck, Shrimp Tempura, Christmas Roll</p> ",this);
});

And so on ...

share|improve this answer
add comment

You can do this in one function that takes into account the ID of the element clicked on to determine what HTML to add to the DOM:

//the idea here is to match the number of the tab (#tab1 being number 1) with the data to display for that tab, the blank first index is because there is no #tab0
var tabData = ['', '<p>Here is our menu: Crispy Duck, Shrimp Tempura, Christmas Roll</p>', '<p>We\'re located at: 13352 Minnieville Rd., Woodbridge, VA </p>', '<p>We can be reached at: (555)555-5555.</p>'];

$('#tab1, #tab2, #tab3').click(function() {

    //get the index of the clicked tab via its ID
    var index = parseInt(this.id.replace('tab', ''), 10);

    //update DOM based on the index of the tab
    $("#page").empty().html(tabData[index]);

    //update links, I recommend adding a class to these links so you don't mess with others in the DOM
    $("a").removeClass("active");
    $(this).addClass("active");

});

Here is an updated JSFiddle: http://jsfiddle.net/Q4zVH/3/

You could also select your tabs by some other type of identifier, like a class, which would make this easier to expand.

share|improve this answer
    
someone here suggested an edit, where parseInt(this. was replaced by ~~. jQuery pros, your call... –  Vogel612 Jun 26 at 12:52
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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