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.
//AMOUNT CALCULATOR
            var totalAmount = 0;
            var serviceAmount = 0;

jQuery('.select-service').click(function(){
    var serviceAmount = parseInt(jQuery(this).parent().html().replace(/\D/g,''));
    if(jQuery(this).is('.selected')){
        jQuery(this).removeClass('selected');
        totalAmount -= serviceAmount;
        jQuery('#total-amount').html(totalAmount);
    }else{
        jQuery(this).addClass('selected');
        totalAmount += serviceAmount;
        jQuery('#total-amount').fadeIn('slow').html(totalAmount);
    }
});
//AMOUNT CALCULATOR

is this code efficient and secure? I am PHP dont know if it is required or not to judge.. Please any Help/feed back would be awesome! really trying to better my coding..

share|improve this question

1 Answer 1

up vote 2 down vote accepted

Security:

If you want to check amounts securely, don't do it in the browser alone: you need a server-side validation (e.g. with PHP if it is your language of choice).

Efficiency:

  1. You can use $('...') instead of jQuery('...') to improve the readability and concision (source: jquery.com).
  2. Avoid looking for divs with jQuery every time you need them by assigning them to a variable. In this case, caching $('#total-amount') seems unnecessary, but it will pay off as soon as you call it once more (see jQuery Best Practices for more information).

Thus:

//AMOUNT CALCULATOR
var totalAmount = serviceAmount = 0;
// caching $('#total-amount')
var totalAmountDiv = $('#total-amount');

$('.select-service').click(function(){
    // caching $(this)
    var thisDiv = $(this);
    var serviceAmount = parseInt(thisDiv.parent().html().replace(/\D/g,''));
    if(thisDiv.is('.selected')){
        thisDiv.removeClass('selected');
        totalAmount -= serviceAmount;
        totalAmountDiv.html(totalAmount);
    }else{
        thisDiv.addClass('selected');
        totalAmount += serviceAmount;
        totalAmountDiv.fadeIn('slow').html(totalAmount);
    }
});
//AMOUNT CALCULATOR
share|improve this answer
    
wow thanks man.. but doesnt using jQuery instead of $ prevent conflicts with mootools? –  Dawid van der Hoven Aug 16 '13 at 8:30
    
Yes it does, I just didn't know you used another library. To avoid conflicts, you can also use jQuery.noconflict() or Mootools' Dollar Safe Mode (mootools.net/blog/2009/06/22/the-dollar-safe-mode) –  David M. Aug 16 '13 at 8:45

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.