Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a simple code that just has to check if a checkbox is checked and if so enable some fields that, by default, should be disabled.

I have written the next code which I know can be severely improved. Could anyone tell me how? I tried getting the siblings of the checkbox also but that just caused some troubles.

$(function(){    
$('#amount').attr('disabled','disabled');
            $('#period').attr('disabled','disabled');
            $('#key').attr('disabled','disabled');
            $('#api').change(function(){
                if($('#apiEnabled').is(':checked')){
                    $('#amount').attr('disabled','');
                    $('#period').attr('disabled','');
                    $('#key').attr('disabled','');
                }else{
                    $('#amount').attr('disabled','disabled');
                    $('#period').attr('disabled','disabled');
                    $('#key').attr('disabled','disabled');
                }
            });
});
share|improve this question

migrated from stackoverflow.com Sep 13 '11 at 10:30

5 Answers

It's difficult to answer accurately without seeing your markup, but maybe you can use a multiple selector to alleviate code redundancy. You can also use prop() to enable or disable your elements more easily:

$(function() {
    var $elements = $("#amount, #period, #key");
    $elements.prop("disabled", true);
    $("#api").change(function() {
        $elements.prop("disabled", !$("#apiEnabled").is(":checked"));
    });
});
share|improve this answer

try fallowing way-

$(function(){    
$('#amount').attr('disabled','disabled');
            $('#period , #key').attr('disabled','disabled');
            $('#api').change(function(){
                if($('#apiEnabled').is(':checked')){
                    $('#amount, #period , #key').attr('disabled','');
                }else{
                    $('#amount, #period , #key').attr('disabled','disabled');
                }
            });
});
share|improve this answer

This is my take at it, but if you won't be needing the separate elements all over the place, then take @Frédéric Hamidi's advice and create a single object.

$(function(){
    var $amount     = $('#amount'),     // cache jQuery objects
        $period     = $('#period'),     // to avoid overhad
        $key        = $('#key'),        // of fetching them
        $apiEnabled = $('#apiEnabled'); // all the time
    $amount.prop('disabled', true);
    $period.prop('disabled', true);
    $key.attr('disabled', true);
    $('#api').change(function(){
        if ($apiEnabled.is(':checked')) {
            $amount.prop('disabled', false);
            $period.prop('disabled', false);
            $key.prop('disabled', false);
        } else{
            $amount.prop('disabled', true);
            $period.prop('disabled', true);
            $key.attr('disabled', true);
        }
    });
});
share|improve this answer

You can cache your set of elements to prevent jQuery from looking them up every time you refer to them. You can also group them all into one set because you are applying the same code to all three of them.

prop can be used instead of attr which is new in jQuery 1.6+. prop is better suited to checkboxes and setting things as disabled.

Now that all three elements are grouped, you have one place (the checkboxes variable) that you can add or remove new checkboxes. It also means more succinct code in your change event.

$(function(){

    // Cache the elements
    var checkboxes = $("#amount, #period, #key");

    // Disable them using prop rather than attr (jquery 1.6+)
    checkboxes.prop('disabled', true);

    $("#api").change(function(){

        if($('#apiEnabled').is(':checked')) checkboxes.prop('disabled', false);
        else checkboxes.prop('disabled', true);

    });

});
share|improve this answer
$(function(){    
    var $els = $('#amount, #period, #key');
    $('#api').change(function(){
        var unchecked = !this.checked;
        $els.each(function(){
            $(this).prop('disabled', unchecked);
        });
    });
});

And if applicable change:

var $els = $('#amount, #period, #key');

To:

var $els = $('#api').siblings();


Example Here:

http://jsfiddle.net/BpVkw/

share|improve this answer

Your Answer

 
discard

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