1
\$\begingroup\$

I'm very new to jQuery and JavaScript in general but decided to develop my own code rather than use 3rd party plugins as I really need to learn the language. Would anyone be so kind as to review my code?

I wanted a relatively simple project to start with and decided to build the jQuery front end to a PHP shopping cart I developed.

$(function(){
    $.ajaxSetup ({  
        cache: false  
    });

    $("#the-items").each(function() {

        $("button").click(function(){

            var basketTotal = $('#cart-total').html();
            var basketItems = $('.link-span').html();
            var button = $(this).attr("name");
            var itemId = $(this).attr("value");
            var itemPrice = $(this).siblings(".item-sing-val").html();
            var quantity = $(this).siblings(".item-qty").html();
            var subTotal = $(this).siblings(".item-val").html();
            var ajaxLoad = "<img src='img/load.gif' alt='loading...' />";

            $(this).load("ajax.php", {row: itemId,task: button}, function(){  

                if (button == 'delete'){
                    updTotalPrice = parseFloat(basketTotal) - parseFloat(subTotal);
                    updBasketItems = parseInt(basketItems) - parseInt(quantity);

                    $(this).parent(".item-row").fadeOut('slow',function(){
                        $(this).remove();
                    });

                } else if (button == 'minus' && quantity > 1) {

                    quantity--;
                    updBasketItems = parseInt(basketItems) - 1;

                    updTotalPrice = parseFloat(basketTotal) - parseFloat(itemPrice);
                    updItemPrice = parseFloat(subTotal) - parseFloat(itemPrice);

                } else if (button == 'plus') {

                    quantity++;
                    updBasketItems = parseInt(basketItems) + 1;

                    updTotalPrice = parseFloat(basketTotal) + parseFloat(itemPrice);
                    updItemPrice = parseFloat(itemPrice) + parseFloat(subTotal);
                }

                $('#cart-total').html(parseFloat(updTotalPrice).toFixed(2));
                $('.link-span').html(updBasketItems);
                $(this).siblings(".item-qty").html(quantity);
                $(this).siblings(".itemval").html(parseFloat(updItemPrice).toFixed(2));
            });    

            return false;
        });    
    });
});

My HTML within the cart is:

<!--This DIV contains the static info about the Basket-->
<div id='the-basket'>
    <span class="font-medium">Basket - &pound;</span>
    <span id='cart-total' class='font-medium'>2.99</span>
    <img class='link-image' src='./images/basket_wedge.png' alt='' />
    <span class='link-span fontwhite'>1</span>
</div>

<!--This DIV contains the dynamic cart line items-->
<div id="the-items">
    <form name='checkout' action='' method='post'>
        <div class='item-row'>

            <!--FLOAT LEFT-->
            <span class='item-name'>The Baconator</span>
            <span class='item-weight'>(180g)</span>

            <!--FLOAT RIGHT-->
            <button type='submit' name='delete' value='0' class='item-delete'></button>

            <span class='item-val'>2.99</span><span class='item-currency'>&pound</span>
            <button type='submit' name='plus' value='0' class='item-plus'></button>
            <span class='item-qty'>1</span>

            <button type='submit' name='minus' value='0' class='item-minus'></button>
            <span class='item-sing-val' style='display:none;'>2.99</span>

        </div>
        <div class='the-links'>
            <ul>
                <li>
                    <input type='submit' name='checkout' value='Checkout' />
                </li>
                <li>
                    <input type='submit' name='empty' value='Clear' />
                </li>
            </ul>
        </div>
    </form>
</div>

The PHP Script at the moment creates a cart object from the cart variable I have in a session. The script is fully functional but lacks any finesse / error handling (I understand I would need to use the .ajax() function if I needed complex error handling, could someone set me straight on this?)

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

Apart from what @jfriend00 and @NachoSoto have said:

  1. Make sure to use === whenever possible, since it doesn't do type coercion and avoids bads surprises (I think the only correct instance of == is when you get an integer value as a string from your html and want to avoid using parseInt)
  2. Consider using a switch statement over the value of button. (And test if the quantity > 1 inside the switch statement.)

You could also consider that droping the quantity to zero is the same than removing the item. This is easy with a switch and no break in the 'minus' and 'quantity == 0' case:

switch(button) {
    case 'minus':
        if (quantity > 1) {
            quantity--;
            updBasketItems = parseInt(basketItems) - 1;

            updTotalPrice = parseFloat(basketTotal) - parseFloat(itemPrice);
            updItemPrice = parseFloat(subTotal) - parseFloat(itemPrice);
            break;
        }
    case 'delete':
        updTotalPrice = parseFloat(basketTotal) - parseFloat(subTotal);
        updBasketItems = parseInt(basketItems) - parseInt(quantity);

        $(this).parent(".item-row").fadeOut('slow',function(){
            $(this).remove();
        });
        break;
    case 'plus':
        quantity++;
        updBasketItems = parseInt(basketItems) + 1;

        updTotalPrice = parseFloat(basketTotal) + parseFloat(itemPrice);
        updItemPrice = parseFloat(itemPrice) + parseFloat(subTotal);
        break;
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the comments, would I be right in thinking that evaluating Switch statements in JS is faster than evaluating if else as it is in PHP? I'll start using explicit type checks as well, that makes alot of sense. Thank you. \$\endgroup\$ – David Barker Feb 29 '12 at 14:03
  • \$\begingroup\$ Actually, there is no significant performance difference between switch() and if else in any language. \$\endgroup\$ – Quentin Pradet Feb 29 '12 at 14:15
3
\$\begingroup\$

Some suggestions:

  1. In most places you are retrieving .html(), I think you really want .text().
  2. You don't want to be using $(this) over and over again because you keep making a new jQuery object over and over again. Evaluate it once into a local variable are refer to that from then on: var this$ = $(this);. Then refer to this$ whenever you want it. But don't get confused because not all values of this are the same in your code so some places will need their own $(this).
  3. Declare all local variables like var updTotalPrice, updBasketItems, updItemPrice; so they are not implicit global variables.
  4. I think you need to convert quantity to a number in quantity = $(this).siblings(".item-qty").html(); because you use it in numeric comparisons later which won't work properly unless you make it into a real number: var quantity = parseInt($(this).siblings(".item-qty").text(), 10);
  5. Your ajax has me baffled. You're loading ajax.php into the clicked button? Is that really what you want to do with this line $(this).load("ajax.php", {row: itemId,task: button}, function(){? That doesn't make a whole lot of sense to me. What are you trying to accomplish with your ajax call?
  6. You don't seem to ever be using var ajaxLoad = "<img src='img/load.gif' alt='loading...' />";.
\$\endgroup\$
  • \$\begingroup\$ I'm curious, do you have a reference saying that using $(this) over and over again is a performance issue? \$\endgroup\$ – Quentin Pradet Feb 29 '12 at 7:21
  • \$\begingroup\$ @Cygal - just step through the creation of a jQuery object in the debugger and you can see what's involved. It's several function calls, the creation of a new object and some code. It's faster to avoid running the same code over and over again. Whether that speed difference is significant to you depends upon your specific situation, but I think it's a good coding habit to not keep repeating the same function call with the same parameters to generate the same result over and over again in a given block of code. \$\endgroup\$ – jfriend00 Feb 29 '12 at 13:53
  • \$\begingroup\$ Thanks for all the suggestions! I tried a load of different methods to get the ajax call to succeed whilst still keeping the button var in scope of the AJAX callback function. Appreciate it probably isn't the right way to do it, some guidance on this would be much appreciated! Thanks for setting me straight on the use of .html() and .text(). No I'm not using the var ajaxLoad in this script, I still need to integrate it. \$\endgroup\$ – David Barker Feb 29 '12 at 14:08
2
\$\begingroup\$

Apart from what @jfriend00 has said, this line is not needed:

$("#the-items").each(function() {

You can use $.each to iterate though a set of elements, but you're selecting a unique element with its ID.

Another advice I would give you to clean up the code is to put all variable declarations (or at least those that share something in common) after the same "var" keyword, like this:

    var basketTotal = $('#cart-total').html(),
        basketItems = $('.link-span').html(),
        button = $(this).attr("name"),
        itemId = $(this).attr("value"),
        itemPrice = $(this).siblings(".item-sing-val").html(),
        quantity = $(this).siblings(".item-qty").html(),
        subTotal = $(this).siblings(".item-val").html(),
        ajaxLoad = "<img src='img/load.gif' alt='loading...' />";
\$\endgroup\$
  • \$\begingroup\$ Thanks Nacho. I'm a bit confused by what you mean with $.each instead of the line of code you highlighted. How would you use that and achieve the same results? Thank you for the code cleanup recommendations! \$\endgroup\$ – David Barker Feb 29 '12 at 14:11
  • \$\begingroup\$ I disagree that .each( line is unnecessary. It's a "functional" solution to needing to check if the selector actually matched something. \$\endgroup\$ – RoToRa Feb 29 '12 at 16:03
1
\$\begingroup\$

There are two things I'd change. The first is a possible bug:

$("#the-items").each(function() {
    $("button").click(function(){

Here the $("button") selector, selects all buttons in the whole page, not only within #the-items. You'll either need to add a context to $("button"):

 $("#the-items").each(function() {
     $("button", this).click(function(){

Or leave out $("#the-items").each( altogether, since it has no other purpose, and use

$("#the-items button").click(function(){

instead.

Regarding the if (or switch as @Cygal suggests) for the different buttons, I'd suggest to do it completely differently, in a more JavaScript/functional way:

var buttonAction = function(whatToDo) {
    return function() {
        var basketTotal = $('#cart-total').html();
        // etc...

        $(this).load("ajax.php", {row: itemId,task: button}, function(){  

            whatToDo(); // It may make sense to pass certain values as arguments here

            $('#cart-total').html(parseFloat(updTotalPrice).toFixed(2));
            // etc....
        });    

        return false;
    };
}

$("#the-items").each(function() {
    $("button[name='delete']", this).click(buttonAction(function(){
        updTotalPrice = parseFloat(basketTotal) - parseFloat(subTotal);
        updBasketItems = parseInt(basketItems) - parseInt(quantity);

        $(this).parent(".item-row").fadeOut('slow',function(){
            $(this).remove();
        });

    });

    $("button[name='minus']", this).click(buttonAction(function(){
        if (quantity > 1) {
            // ...
        }
    });


    $("button[name='plus']", this).click(buttonAction(function(){
        // ...
    });
});
\$\endgroup\$

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.