2
\$\begingroup\$

How can I make this code shorter?

function tblcheckboxes(){
    var a= 0;
    var b= 0;
    var c= 0;
    var d= 0;
    var e= 0;
    var f= 0;
    var g= 0;

    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);
    }
    if ($('#tcbx6').is(":checked")) {
         f = parseFloat($("#tcbx6").val(), 10);
    }
    if ($('#tcbx7').is(":checked")) {
         g = parseFloat($("#tcbx7").val(), 10);
    }

    var total = a + b + c + d + e + f + g;
    $('.txt7').val(total.toFixed(2)); 
}

$(document).ready(function(){
        $('#tcbx1').click(function(){
                tblcheckboxes();
                grandtotal();
        });
        $('#tcbx2').click(function(){
                tblcheckboxes();
                grandtotal();
        });        
        $('#tcbx3').click(function(){
                tblcheckboxes();
                grandtotal();
        });
        $('#tcbx4').click(function(){
                tblcheckboxes();
                grandtotal();
        });
        $('#tcbx5').click(function(){
                tblcheckboxes();
                grandtotal();
        });
        $('#tcbx6').click(function(){
                tblcheckboxes();
                grandtotal();
        });
        $('#tcbx7').click(function(){
                tblcheckboxes();
                grandtotal();
        });
});
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I'm sure there is a way to simplify it. I hope you get some good reviews! \$\endgroup\$ – Phrancis Jan 8 '15 at 1:47
3
\$\begingroup\$

You could try to do the whole thing on the fly, like this:

var total = 0;
$("input.tcbx").change(function(){
    if($(this).is(":checked")) {
        total += parseFloat($(this).val(), 10);
    } else {
        total -= parseFloat($(this).val(), 10);
    }
    $('.txt7').val(total.toFixed(2)); 
});

Just add a class to all your checkboxes, and then either add the value to your total or reduce it as they get selected. If you have preselected values you might want to enter the preselected total already, otherwise you could use an each() to first check all the values.

$(document).ready(function(){
    var total = 0;
    $("input.tcbx").change(function(){
    	if($(this).is(":checked")) {
    		total += parseFloat($(this).val(), 10);
    	} else {
    		total -= parseFloat($(this).val(), 10);
    	}
    	$('#result').val(total.toFixed(2)); 
    });
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>
<input type="checkbox" value="1" id="check-1" class="tcbx" />
<label for="check-1">1</label>
<input type="checkbox" value="2" id="check-2" class="tcbx" />
<label for="check-2">2</label>
<input type="checkbox" value="3" id="check-3" class="tcbx" />
<label for="check-3">3</label>
<input type="text" id="result">0</div>

\$\endgroup\$
1
\$\begingroup\$

For your document.ready function, I recommend using a loop, rather than going through each individual element:

$(document).ready(function() {
    for(var i = 1; i <= 7; i++) {
        $("#tcbx" + i).click(function() {
            tblcheckboxes();
            grandtotal();
        });
    }
});

And the same goes to your tblcheckboxes function, along with a better way to initialize, store, and add your variables:

function tblcheckboxes() {
    var a = b = c = d = e = f = g = 0;
    var vars = [a, b, c, d, e, f, g]; // store in an array for easier access

    var total = 0; // initialize total so it can be added while looping through the elements

    for(var i = 1; i <= 7; i++) {
        if($("#tcbx" + i).is(":checked")) {
            vars[i - 1] = parseFloat($("#tcbx" + i).val(), 10);
        }
        total += vars[i - 1];
    }

    $(".txt7").val(total.toFixed(2));
}
\$\endgroup\$
1
\$\begingroup\$

You can simplify and shorten code by using a container element for your checkboxes and just use one click handler on that (using jquery event delegation). Within the handler you can calculate the total for all checked checkboxes values. The snippet is a demo for it.

In it, the handler:

  • gets the collection of all checked checkboxes within the container
  • converts them to an array
  • uses the Array.reduce method to calculate the total
  • displays the calculated total within div#total

The advantage for this approach is also that you can add checkboxes dynamically, which are included immediately in the calculations. The second handler in the snippet demonstrates that.

$('#pricing').on( 'click', '[type=checkbox]', function (e) {
    // all checked checkboxes in #pricing
    var totamount = $('#totalamount').hide();
    var total = $('#pricing > [type=checkbox]:checked') 
                .toArray()                       //<= convert to array 
                .reduce( function (a, b) {       //<= calculate sum
                          return a + (+b.value); //<= + operator converts to Number
                         },0 );
    totamount.html( total.toFixed(2) ).fadeIn();
});

$('#addcbx').on('click', function (e) {
    var amount = $('#cbvalue').val();
    if (!amount || amount < 0.001) {
      return true;
    }
    var nwcbx = $('<input type="checkbox" value="' + amount + '" \/> '+ amount+ '<br>')
                  .insertBefore($('#total'));
});
body {
  margin: 2em;
  font: 0.8em/1em normal verdana, arial;
}

#total {
  width: 150px;
  border-top: 1px solid #ddd;
  padding-top: 5px;
}

#totalamount {
  font-weight: bold;
  color: green;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div id="pricing">
  <input type="checkbox" value=100.00 /> 100.00<br>
  <input type="checkbox" value=231.00 /> 231.00<br>
  <input type="checkbox" value=521.99 /> 521.99<br>
  <input type="checkbox" value=22.25 /> 22.25<br>
  <input type="checkbox" value=36.55 /> 36.55<br>
  <input type="checkbox" value=96.32 /> 96.32<br>
  <input type="checkbox" value=5.50 /> 5.50<br>
  <div id="total">total: <span id="totalamount">0</div>
</div>

<p>
  <input type="number" id="cbvalue" placeholder="amount" step="0.01" min="0.00" value="0.00"/>
  <button id="addcbx">add checkbox for amount</button>
</p>

\$\endgroup\$
1
\$\begingroup\$

I would not use IDs, but a identifier for all your checkboxes, a class, obviously.

And I would store each values in an array instead of many vars.

$(function() {
  // The collections of elements :
  var $tcbxCollection = $('.tcbx');

  // get the values of this collection :
  var tcbxValues = getCheckboxesValues($tcbxCollection);

  $('body' /* or the parent container */ ).on('click', function() {
    tcbxValues = getCheckboxesValues($tcbxCollection);
    console.log(tcbxValues);
  });

  // The function to put the important values.
  function getCheckboxesValues($collection) {
    var checkboxValues = [];

    $collection.each(function(i, element) {
      checkboxValues[i] = $(element).is(':checked') ?
        parseFloat($(element).attr('value'), 10) : 0;
    });

    return checkboxValues;
  }

});
<input class="tcbx" type="checkbox" value="10.66666" />
<input class="tcbx" type="checkbox" value="28.45368789" />
<input class="tcbx" type="checkbox" value="32.5" />
<input class="tcbx" type="checkbox" value="4.5456" />
<input class="tcbx" type="checkbox" value="54.786" />
<input class="tcbx" type="checkbox" value="65" />
<input class="tcbx" type="checkbox" value="7.56464" />
<input class="tcbx" type="checkbox" value="8.7789" />
<p>Look on the console...</p>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>

\$\endgroup\$
  • 1
    \$\begingroup\$ I do have to add that this will mean that whenever a user clicks, the function will run. It's best practise to only do it when something actually happens, so maybe your $('body').click should be $tcbxCollection.click or $tcbxCollection.change, and not a parent.click to save your resources. Only calculate when something happens, thats why I used the change() event, it is tailor-made for this. \$\endgroup\$ – somethinghere Jan 9 '15 at 17:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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