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.

If a checkbox is clicked, it will be added to the total, if not, it will be deducted or ignored. (The total number of checkboxes I have in real is 25.)

Is there a way to write the code below in a shorter version?

Function

function tblcheckboxes(){
    var a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, q, r, s, t, u, v, total;
    if ($('#tcbx1').is(":checked")) {
         a = parseFloat($("#tcbx1").val(), 10);
    }
    if ($('#tcbx2').is(":checked")) {
         b = parseFloat($("#tcbx2").val(), 10);
    }
    if ($('#tcbx3').is(":checked")) {
         c = parseFloat($("#tcbx3").val(), 10);
    }
    if ($('#tcbx4').is(":checked")) {
         d = parseFloat($("#tcbx4").val(), 10);
    }
    if ($('#tcbx5').is(":checked")) {
         e = parseFloat($("#tcbx5").val(), 10);
    }

jQuery

$(document).ready(function(){
    $('#tcbx1').click(function(){
            tblcheckboxes();
    });
    $('#tcbx2').click(function(){
            tblcheckboxes();
    });
    $('#tcbx3').click(function(){
            tblcheckboxes();
    });
    $('#tcbx4').click(function(){
            tblcheckboxes();
    });
    $('#tcbx5').click(function(){
            tblcheckboxes();
    });
   });
share|improve this question
    
Is it really a good idea to be storing data in a checkbox value? –  moarboilerplate Dec 23 '14 at 21:22
    
@moarboilerplate - that sounds like a different question to me, but yes, it's fine. That's what the value field is for. –  Jasmine Dec 23 '14 at 22:14

5 Answers 5

Why not do something like this:

$(document).ready(function() {
     $('.containerClass-filter-expression-that-selects-all-your-chks').click(function() {
       recalculate();
     });
});

function recalculate(){
   $('.some-expression-that-selects-all-your-checkboxes').each(function() {
       if($(this).is('checked')) {
           //add

       } else {
           //substract from total
       }

   });
};
share|improve this answer
    
And what Jasmine says. You can seperatly group select your checked and unchecked boxes, depending on your needs. –  Danthar Dec 23 '14 at 18:53

I'm assuming you have an interface with multiple check boxes, and some indicator of the number currently checked... I do this all the time. There's an important thing you need to keep in mind - you can't do this by responding to click events of the checkbox, IF you have other ways that it can be checked, such as by a script, or if the checkbox might be rendered checked when the page loads. So, I create a counting function, and I call that whenever anything happens. When my script updates the boxes, I call the function. When the page loads, I call the function. I also add the function as the click event responder on all checkboxes, including my "check all" box. The function is very simple...

All my checkboxes have the class "gridCheckbox" and the function counts those, if they are checked, and puts the result into the label "lblNumSelected"...

function UpdateCheckCount() {
    $("#lblNumSelected").html("(" + $(".gridCheckbox:checked").length + ")");
}

Your code shows a misunderstanding of JQuery and what it's for. It is a Query Language over the DOM. As such, you should try to apply "group logic" whenever possible, and NEVER iterate over individual items. So, rather than writing code for each check box, you write code for ALL checkboxes at the same time. You should operate on groups (query result sets) rather than individual items, as much as possible.

share|improve this answer

You can list multiple dom selectors separated by commas, for example:

$('#tcbx1, #tcbx2, #tcbx3').click(function(){
        tblcheckboxes();
});

And since the anonymous function simply calls another function, you can just pass in the function directly:

$('#tcbx1, #tcbx2, #tcbx3').click(tblcheckboxes);

As for this part:

if ($('#tcbx1').is(":checked")) {
     a = parseFloat($("#tcbx1").val(), 10);
}
if ($('#tcbx2').is(":checked")) {
     b = parseFloat($("#tcbx2").val(), 10);
}

This is trickier, because although the operations inside are very similar, you are assigning to different variables. A workaround could be to assign to array elements instead of a, b, c, where the right element index could be extracted from the number in the #tcbxN id. Then you could use a loop, creating id names as "#tcbx" + i and using the loop variable i to assign values into the array. But that still wouldn't be great.

To be honest, this code with variable names like a, b, c look quite hypothetical. If you presented a real life problem with well-named variables, we might be able to provide better big-picture suggestions to shorten the code.

share|improve this answer

I tried some of the answers,did not work for me. I played around a bit on JSFiddle, to come up with the below code. Your JQuery selector can change as per the structure of the HTML page. Also,note it's is(':checked')

var count=0;
    $("input[type='checkbox']").on('click',function(e){
        if($(this).is(':checked'))
           count+= parseInt($(this).val());
        else
            count-= parseInt($(this).val());
        console.log(count)
    });

Src : http://jsfiddle.net/sahiljambhekar/8498q9ox/2/

share|improve this answer
    
My personal preference is to make the total inside the scope of the function because otherwise you have to "unwind" the previous calls by subtracting. –  moarboilerplate Dec 23 '14 at 21:26
    
Yep @moarboilerplate : that makes sense. Thanks. –  Sahil J Dec 24 '14 at 16:13
    
and thank you for helping me find a bug in my code! –  moarboilerplate Dec 24 '14 at 16:18

If there are no other checkboxes on the page, just select all checkboxes using $('[type="checkbox"]'). If you can't select all checkboxes, put a class on your checkboxes and select the class by using $('.theCheckboxClass'). If you are a sadist, select all your checkboxes by id e.g. $('#tcbx1, #tcbx2, #tcbx3'). We'll use the checkbox element selector for simplicity. Bind the function to calculate totals to the change event instead of click.

var checkBoxes = $('[type="checkbox"]');
checkBoxes.change(function () {
    var total = 0;
    checkBoxes.filter(":checked").each(function() {
        total += parseFloat($(this).val(), 10);
    });
    DoSomethingWithTotal(total);
});
share|improve this answer

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.