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 came up with this jQuery snippet to add/remove a class on <label> tags that contain either checkboxes or radio buttons, in order to improve usability so users can see what item(s) they have selected.

But I would like to know if there's a way to optimize the code (or not if that's the case).

$('label input[type=radio], label input[type=checkbox]').click(function() {
  $('label:has(input:checked)').addClass('active');
  $('label:has(input:not(:checked))').removeClass('active');
});
share|improve this question

3 Answers 3

up vote 2 down vote accepted

I know you've marked this question answered, but since this website is about 'code review', and further improvements are possible

You're currently attaching event-handlers on each checkbox, which is inefficient, it's far more efficient to use 'delegation', making use of event-bubbling. Reducing the amount of event-handlers is better for performance and reduces the risk for memory-leaks, especially if you're adding/removing elements to your page dynamically.

More information on Event Delegation:

In general: http://davidwalsh.name/event-delegate

jQuery.on(): http://api.jquery.com/on/#direct-and-delegated-events

With jQuery, you can make use of 'delegation' by putting a .on() event handler on a parent element. A performance comparison can be found here http://jsperf.com/jquery-live-vs-delegate-vs-on/34

(although you're not using the .live() function, it is to illustrate using separate event-handlers vs. a single handler/delegation)

Here's some information that explains how it works: http://www.jquery4u.com/jquery-functions/on-vs-live-review/

For your specific example, the code would be like this:

$(document).on('click', 'label input[type=checkbox]').click(function() {
    $(this.parentNode).toggleClass('active', this.checked);
});

This will create a single event-handler that handles all click events on 'checkboxes' if they are wrapped in a label like this:

<label><input type='checkbox' id='check1' value='1' />Check me</label>

However, if you don't 'wrap' your input in a label, but have them separate, using the 'for' attribute:

<input type='checkbox' id='check1' value='1' />  <label for='check1'>check me</label>

Then the code above won't work, in this case you'll have to select the label in a different way:

$(document).on('click', 'input[type=checkbox]').click(function() {
    $("label[for='" + this.id + "']").toggleClass('active', this.checked);
});
share|improve this answer
    
Very well explained, and the link references are a great help. You also addressed different situations (<input>s inside and outside the <label>), which can certainly happen down the road. Did not know about the concept of 'delegation' until now. Thanks a million for all the information. –  Ricardo Feb 1 '13 at 15:22
    
@Ricardo glad I was able to help. Kinda made it my mission to try to educate people a bit. Hoping other people will read and learn as well –  thaJeztah Feb 1 '13 at 15:45
    
If we want to squeeze out every bit of performance, you should replace $(this).attr('id') with simply this.id. –  Joseph Silber Feb 1 '13 at 21:24
    
@JosephSilber good suggestion, I'll modify my answer –  thaJeztah Feb 1 '13 at 21:25

You're adding a click handler to all your inputs, and then implementing the click handler such that each click processes all the labels. This is probably not what you want.

I'd suggest the following:

$('label input[type=checkbox]').click(function() {
    $(this.parentNode).toggleClass('active', this.checked);
});

$('label input[type=radio]').click(function() {
    $('input[name="' + this.name + '"]').each(function(){
        $(this.parentNode).toggleClass('active', this.checked);
    });
});

Happily enough, that approach is also more efficient, as it's processing 1 element per click for checkboxes instead of n elements per click. For radio buttons it still has to process multiple elements, but the number processed is still less than the total number of elements on the page (assuming the page contains more than a single group of radio buttons).

Here's a live example: http://jsfiddle.net/DfU3R/3/

As nickles80 rightly notes, however, the performance difference between the two approaches will likely be negligible, and premature optimization is the root of all evil.

share|improve this answer
    
Although $(elem.parentNode) is better than $(elem).parent() performance-wise, I think you should use the latter. If you're using jQuery, use it properly. –  Joseph Silber Feb 1 '13 at 2:42
    
Also, instead of that bloated applyClass function, just use toggleClass; it takes a second parameter as a boolean: $(this).parent().toggleClass('active', this.checked); –  Joseph Silber Feb 1 '13 at 2:44
    
Well spotted on the second point, it's much more compact using toggleClass(). On the first point, however, I have to disagree. Using jQuery properly doesn't necessarily mean giving it a monopoly on all DOM accesses. –  aroth Feb 1 '13 at 3:31
    
@aroth Yes, as I mentioned on nickles80's answer, if there's a case where it is necessary to have long forms with loads of check boxes and radio buttons, I would like the page to perform as efficiently as possible, which means that your suggested improved snippet seems to do exactly that. Will certainly consider it for the future. Thanks a lot for the jsfiddle demo as well. –  Ricardo Feb 1 '13 at 4:21
    
@Ricardo Read my answer below; For long forms, event delegation is the best approach –  thaJeztah Feb 1 '13 at 9:38

You are probably pretty good here.

You might be able to micro optimize by only highlighting / un-highlighting the corresponding label of the input but unless you have 100s of labels on your form I wouldn't bother. Especially since the radio button implementation would not be simple.

The only caveat I have is that if you have a label with the class active that has an input of any type (textbox) that class will also be removed upon clicking any of the checkboxes. But I doubt that really matters for you.

demo: http://jsfiddle.net/JsUWv/

"Don't optimize until you need to."

share|improve this answer
    
"Don't optimize until you need to."-- Hmm, I would've never thought of this when writing JavaScript; I'm clearly just starting with JS. Nonetheless, yes, when it comes to forms I try to keep them as short as possible, but if there's a case where it is necessary to have long forms with loads of check boxes and radio buttons, I would like the page to perform as efficiently as possible. I think I'll leave what I have as is since I only have a couple of radio buttons and checkboxes on the form I'm implementing this snippet on. Thanks a lot. –  Ricardo Feb 1 '13 at 4:16
    
@nickles80 this website is about 'code review' - try to assist people to get their code as good as possible and correct mistakes or bad practice. Even if you normally wouldn't optimize the code (e.g. the code is part of an admin page that is only used once a month, where performance isn't a big issue) –  thaJeztah Feb 1 '13 at 15:50
    
@thaJeztah That is fair. But he specifically asked if it needed it. I didn't think it did. Optimizing this snippet would take it from 4 lines to probably somewhere around 50. And might be a little faster. Sometimes in reviewing code, the code doesn't need any improvements. –  nickles80 Feb 1 '13 at 16:32

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.