Menu that updates a total price on option click

Question

I'm teaching myself JavaScript and jQuery at the same time by trying to complete little projects that I come up with or my friends come up with. I have no official JavaScript training and am pretty much just OJT (without the job).

I created a function below and was hoping I could get some feedback from the experts and professionals at SO. Could you tell me what I could do better or more efficiently? What is unprofessional in my function? I haven't learned any best practices or had any kind of mentor so I'm not sure what's generally 'acceptable' and what's not. I have a feeling you guys wouldn't like the way I use some arrays in my function for one thing, and anything else you could suggest would be great.

Basically what I'm trying to create, is a price that will auto update on each click of a menu item (checkboxes). There's a starting price, a quantity textbox, most options have an additional charge, some don't. The options are grouped into similar categories, and sometimes you get "a # of included" options in your price, so maybe you have 1 included option in that group, that means you should be able to add 1 without having the price go up, then the next time you add one, it should go up.

The HTML markup for the menu would look something like this:

<p id="Tprice" class="price">10.00</p>
<label for="quantity" class="quantity">QTY</label>
<input id="hidden1" type="hidden" value="1" class="includeds"> // how many options are included
<h3>cool stuff, one included</h3>
<ul class="groups">
    <li><input id="li1" type="checkbox" name="test2_menu" title="1.00" onclick="updatePrice()">cheese</li>
    <li><input id="li2" type="checkbox" name="test2_menu" title="2.00" onclick="updatePrice()">bread</li>
    <li><input id="li3" type="checkbox" name="test2_menu" title="3.50" onclick="updatePrice()">snacks</li>
</ul>
<input id="hidden2" type="hidden" value="0" class="includeds">
<h3>other stuff</h3>
<ul class="groups">
    <li><input id="li4" type="checkbox" name="test1_menu" title="0" onclick="updatePrice()">cake</li>
    <li><input id="li5" type="checkbox" name="test1_menu" title="0.50" onclick="updatePrice()">soup</li>
    <li><input id="li6" type="checkbox" name="test1_menu" title="6.25" onclick="updatePrice()">donut</li>
</ul>

The JavaScript for updatePrice is as follows:

var WKoriginal = '10.00', WKlastPrice = WKoriginal;
// original "no additional options" price for item set
// last price initially set to same as original on page load, updates after each price update to the new price, this way if a free option is selected, we don't do the price update animation unless the number is different

function updatePrice() {  
    var priceControlArray = new Array(), 
    priceControlArrayMax = new Array(), 
    priceControlArrayCount = new Array(), 
    WKextras = 0, 
    WKnewPrice = 0, 
    priceControlArrayIndex = 0;
    // upextras = total amount of additional charges to apply to original price, based on what's checked
    // arrays keep track of groups that have a number of additional charges included in original price

    $('.checkbox').each(function() { // for each item with class 'checkbox'
    if ($(this).is(':checked')) { // if the item is checked, else do nothing
        if($(this).closest('ul').prevAll('.includeds:first').val() !== '0') {  
        // if the group has a number of included additionals (based on hidden field above each group), else just add the price to upextras
        var indexofArray = -1;  
        for (i=0; i < priceControlArray.length; i++) {
            if (priceControlArray[i] === $(this).attr('name').split('_')[0]) {
            // search array for an entry matching groupname
            indexofArray = i; 
            break; 
            }
        }
        if (indexofArray === -1) { 
        // if group wasnt found in array, add it, else work with existing array entry
            priceControlArray[priceControlArrayIndex] = $(this).attr('name').split('_')[0]; // create array line with group name
            priceControlArrayMax[priceControlArrayIndex] = $(this).closest('ul').prevAll('.includeds:first').val(); // create array line with group included value
            priceControlArrayCount[priceControlArrayIndex] = 1;  // create array line for group that says 1 item has been added so far
            priceControlArrayIndex = parseInt(priceControlArrayIndex) + 1; // bump up array index for next potential new group
        } else {
        // work with existing array entry
            if (parseInt(priceControlArrayMax[indexofArray]) <= parseInt(priceControlArrayCount[indexofArray])) {
            // if the number of included options for group is less than or equal to the current amount of group's checked options, add price to upextras, else do not add
            WKextras = parseFloat(WKextras) + parseFloat($(this).attr('title')); // add price to upextras
            priceControlArrayCount[indexofArray] = parseInt(priceControlArrayCount[indexofArray]) + 1; // bump up number of included options for group by 1
            } else {
            priceControlArrayCount[indexofArray] = parseInt(priceControlArrayCount[indexofArray]) + 1; // bump up number of included options for group by 1
            }
        }
        } else {
        WKextras = parseFloat(WKextras) + parseFloat($(this).attr('title')); // add price of checked option to upextras if there wasn't any group with included # > 0 attached to it
        }
    }
    });

    WKnewPrice = (parseFloat(WKoriginal) + parseFloat(WKextras)) * parseFloat($('#quantity').val()); // add original price, total extra charges and multiply by quantity count

    if (parseFloat(WKnewPrice) !== WKlastPrice) {
    // if new price is different from the last price, animation price change
    $('#Tprice').fadeOut('fast', function() {
    // fadeout existing price, after completed, fade in new price
        $("#Tprice").html(WKnewPrice.toFixed(2).toString()).fadeIn(); // format price to XX.XX
    });
    }
    WKlastPrice = WKnewPrice; // update last price, for comparison on next run through
}

The function works as you see it now, but I'm hoping the pros here can point out what I'm doing badly or inefficiently so I can learn good habits early.

Answer 1

On your markup:

you have id's for all your <input> elements. Seems unnecessary to have id's unless you need them.

    <li><input id="li1" ...
    <li><input id="li2" ...
    <li><input id="li3" ...

    <li><input id="li4" ...
    <li><input id="li5" ...
    <li><input id="li6"...

With your label:

<label for="quantity" class="quantity">QTY</label>

the 'for=""` attribute should contain the id of an element (probably an input element). You don't seem to have one called quantity. I'm not sure if this was a typo or not.

you are using the title="" attribute for keeping values. Don't. You should use a data-* attribute .

title="0"

becomes

data-price="0"

then in jQuery you can easily access it by

$(myelement).data("price")

also you are using arrays to hold your elements. It would be easier and quicker to put a class on them and access them in code by that class. You can already access them by

$(".groups input")

put that in a variable instead of making an array.

---

Just as a note I'd suggest you read up on css selectors as that helps a lot with jQuery:

• http://htmldog.com/guides/cssbeginner/
• http://htmldog.com/guides/cssintermediate/
• http://htmldog.com/guides/cssadvanced/

Answer 2

here are some code optimization ( had not checked the algorithm , only syntax ).

#1 This :

var priceControlArray = new Array(), 
priceControlArrayMax = new Array(), 
priceControlArrayCount = new Array(), 
WKextras = 0, 
WKnewPrice = 0, 
priceControlArrayIndex = 0;

May be replaced with this :

var priceControlArray = priceControlArrayMax = priceControlArrayCount = [] , 
WKextras = WKnewPrice = priceControlArrayIndex = 0;

#2 Don't call $(this) all the time, like here :

 if ($(this).is(':checked')) { // if the item is checked, else do nothing
    if($(this).closest('ul') ...

Save it at the start of the function and use cached variable :

var $this = $(this); //and then use $this variable, it much faster
if ( $this.is(':checked') ) { // if the item is checked, else do nothing
        if( $this.closest('ul') ...

#3 The names of you variables are too long , it is kind hard to read, but this is not an issue, it depends on your own code style. I suggest you to read the source code of jquery library - good example of syntax order.

#4 Dont use onclick="updatePrice() remove it from HTML and attach this event from jquery

$(document).ready(function() {
    $("#li1 , #li2, #li3, #li4, #li4, #li5, #li6").bind( "click" , updatePrice );
});

Better not to use div's id , I suggest you to add a common class and then bind the event to this class only

$(document).ready(function() {
    $(".prices").bind( "click" , updatePrice );
});