2
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ It's only been 17 minutes, and you posted quite a bit of code. \$\endgroup\$ – DeadMG Oct 25 '11 at 14:48
  • \$\begingroup\$ alrighty sorry all \$\endgroup\$ – user1012853 Oct 25 '11 at 14:49
3
\$\begingroup\$

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:

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

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 );
});
\$\endgroup\$
1
  • \$\begingroup\$ var priceControlArray = priceControlArrayMax = priceControlArrayCount = [] , is very bad for objects/lists (or anything that is not a literal value). because they all point to the same list. Change one and they all change. \$\endgroup\$ – James Khoury Oct 30 '11 at 22:30

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.