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.

http://duc.pagodabox.com/product.php

I'm developing a JS based app that allows you to customize a product. I still consider myself a super newbie to javascript and especially to object oriented programming. So far, so good, the app does what I want it to do, so no trouble there.

My background is mostly in jQuery, so my main concern is how I'm sharing and re-using variables between functions, and how I can make this code prettier and more maintainable through optimizing those functions and vars.

Any tips and how to make this more semantic, more efficient, and cleaner code, I would really appreciate!

If you want to really put some time into giving feedback, I can trade my own hours in exchange for a little education!

thanks!

A little more info. The point of the app is:

  1. Click a product option
  2. Select a finish type
  3. Select a finish color
  4. If there is an additional price for that finish, update the price

update if you've already looked at this code: I just ran it through jshint.com and corrected some of the minor bracket and semicolon issues. Thanks!


UPDATE! I've made alot of positive progress and am again reposting new code. I broke up what was once one big file into 3 modules. And am loading them using LABjs. If anyone has any comments on loading efficiency / module loading, etc. I'd really appreciate that as well!

$LAB
    .script(homeUrl + "/assets/js/product/product.js").wait()
    .script(homeUrl + "/assets/js/product/product-color.js")
    .script(homeUrl + "/assets/js/product/product-events.js")

First file PRODUCT.JS

(function (window, document, $) {
    "use strict";

    // Set some general element variables used throughout this script
    var $nodes = {
        list: $('.cust-list'),
        steps: $('.steps'),
        step: $('.cust-step'),
        stepsSlide: $('.steps-slide'),
        subtotal: $('.customizer .total .price'),
        allTypes: $('.finish-type a'),
        allColors: $('.finish-color a'),
        options: $('.cust-option'),
        checks: $('.cust-option-checklist a')
    };

    function Product (name) {
        this.name = name;
        this.options = [];
    }

    // Loops through the options slide divs
    function optionsLoop($options, callback) {
        for(var i = 0; i < $options.length; i++) {
            callback(i, $options[i]);
        }
    }

    // Loops through the array of product options within the product object
    function productOptionsLoop(productOptions, callback) {
        for(var i = 0; i < productOptions.length; i++) {
            callback(i, productOptions[i]);
        }
    }

    // Populate the product object with an array of options
    function getProductOptions($nodes, product) {
        optionsLoop($nodes.options, function(index,value) {
            var $me = $(value),
                name = $me.attr('data-option'),
                type = $me.attr('data-option-type'),
                option = {
                    option: name,
                    type: type
                };

            product.options.push(option);
        });
    }

    // Change the cost according to the added options / variations
    function updateCost(addCost, productOptions, totalPrice, $subtotal, productPrice, $nodes) {
        var currentSubtotal = $subtotal.attr('data-subtotal');

        addCost = 0;

        // Go through all the product options, if an additional cost has been set, add them up
        productOptionsLoop(productOptions, function(index,value){
            var $me = value,
                cost = $me.cost;

            if(cost) {
                addCost += cost;
            }
        });

        productPrice = +productPrice;
        totalPrice = productPrice + addCost;

        animateNumber($nodes.subtotal, currentSubtotal, totalPrice);

        // Update the data attribute on the subtotal to reflect the user's choices
        $nodes.subtotal.attr('data-subtotal',totalPrice);

        // animating number produces rounding errors, so shortly after we animate, update the text with the exact total
        setTimeout(function(){
            $nodes.subtotal.text(totalPrice).digits();
        },325);
    }

    function updateOptions(productOptions, myOption, myName, myCost, myType) {
        // Go through the array of options and add the selected color and cost to this option
        productOptionsLoop(productOptions, function(index,value) {
            var $this = value;
            if($this.option === myOption){
                $this.name = myName;
                $this.cost = Math.floor(myCost);
                if(myType) {
                    $this.type = myType;
                }
                return false;
            }
        });
    }

    $.extend(window, {
        '$nodes': $nodes,
        'Product': Product,
        'optionsLoop': optionsLoop,
        'productOptionsLoop': productOptionsLoop,
        'getProductOptions': getProductOptions,
        'updateCost': updateCost,
        'updateOptions': updateOptions
    });
}(window, document, jQuery));

Second file PRODUCT-COLOR.JS

$(document).ready(function(){
    var productName = $('#product').attr('data-product-name'),
        // Create a new product object with the name of the currently viewed product
        product = new Product(productName),
        // This is set to true when the slide animation has completed, to avoid multiple fast clicks
        animReady = true,
        productPrice,
        totalPrice,
        productOptions,
        addCost,
        inAnim = {},
        outAnim = {opacity: 0},
        outCss = {opacity: 1};

    getProductOptions($nodes, product);
    productOptions = product.options;

    productPrice = $nodes.subtotal.attr('data-subtotal');

    // Color selecting
    $nodes.checks.add($nodes.allColors).add($nodes.allTypes).on('click',function(){
        if($(this).hasClass('current')) {
            return false;
        }

        var $me = $(this),
            $parent = $me.parent(),
            $granpa = $me.parents('.cust-step'),
            granpaEq = $granpa.index() - 1,
            myOption = $granpa.attr('data-option'),
            $myCheck = $('.option-list li:eq(' + granpaEq + ')'),

            myCost = $me.attr('data-option-cost'),
            myName = $me.attr('data-option-name'),
            myType = null,

            $optTypes,
            $optColors,
            $curColor,
            $curType,
            $myParentType,
            $myColors,

            className,
            $add,
            $remove,

            isCheck = $me.is('.cust-option-checklist a'),
            isColor = $me.is('.finish-color a'),
            isType = $me.is('.finish-type a');

        if(isCheck) {
            var $curCheck = $granpa.find('a.selected');

            className = 'selected';
            $add = $me;
            $remove = $curCheck;
        }

        if(isColor || isType) {
            if(isColor) {
                // If we're clicking a color, select the <a> links
                myType = $parent.attr('data-finish-type');
                $optColors = $granpa.find('.finish-color a');
            } else {
                // If we're clicking a color, select the divs containing each color <a>
                myType = $me.attr('data-finish-type');
                $optColors = $granpa.find('.finish-color');
            }

            // All types and colors for the current option
            $optTypes = $granpa.find('.finish-type a');

            $curColor = $optColors.filter('.current');
            $curType = $optTypes.filter('.current');

            if(isColor) {
                var myBg = $me.css('backgroundColor'),
                    myBgImg = $me.css('backgroundImage'),
                    bg = myBgImg;

                $myParentType = $optTypes.filter('[data-finish-type=' + myType + ']');
                $remove = $curColor.add($optTypes);
                $add = $me.add($myParentType);
                className = 'current ic-check selected';
            } else {
                $myColors = $optColors.filter('[data-finish-type=' + myType + ']');

                className = 'current';
                $remove = $curColor.add($curType);
                $add = $me.add($myColors);
            }
        }

        // Add selected class to chosen finish + type
        setCurrent($add,$remove,className);

        if(isColor) {       
            $curType = $optTypes.filter('.current');

            // Set the background image for parent type to reflect chosen color
            if(myBgImg === 'none') {
                bg = myBg;
            }

            $curType.css({'background' : bg});
        }

        // If you select a color or a checkbox, mark the list item as selected
        if( (isColor || isCheck) && !$myCheck.hasClass('.selected') ) {
            $myCheck.add($granpa).addClass('selected');
        }

        updateOptions(productOptions, myOption, myName, myCost, myType);
        updateCost(addCost, productOptions, totalPrice, $nodes.subtotal, productPrice, $nodes);

        // Remove existing price indicator
        $myCheck.find('.price').remove();

        // Add price indicator to checklist if there is an extra cost, or else
        if(myCost > 0) {
            $myCheck.addClass('extra').find('a').append('<span class="f9 price">$' + myCost + '</span>');
        } else {
            $myCheck.removeClass('extra');
        }
    });

    // Navigation
    $('.cust-btn:not(.go-back)').on('click',function(){
        var $me = $(this),
            $curStep = $nodes.step.filter('.cust-step-cur'),
            $nextStep = $curStep.next(),
            $prevStep = $curStep.prev(),
            isPrev = $me.hasClass('prev'),
            $tar = $nextStep,
            curIndex,
            offset,
            speed = 350;

        if(isPrev) {
            $tar = $prevStep;
            if($tar.length === 0) {
                $tar = $nodes.step.filter(':last');
            }
        } else {
            if($tar.length === 0) {
                $tar = $nodes.step.filter(':first');
                speed = 0;
            }
        }

        setCurrent($tar, $curStep, 'cust-step-cur');

        $curStep = $nodes.step.filter('.cust-step-cur');
        curIndex = $curStep.index('.cust-step');
        offset = curIndex * 160;

        $nodes.stepsSlide.animate({
            right: -offset
        },speed);
    });

    // Checklist Click
    $('.option-list a').on('click',function(){
        var $me = $(this),
            myIndex = ($me.parent().index()) + 1,
            $mySlide = $nodes.step.eq(myIndex),
            offset = myIndex * 160;

        setCurrent($mySlide, $nodes.list, 'cust-step-cur');

        $nodes.stepsSlide.animate({
            right: -offset
        }, 0);
    });

    $('.cust-btn.go-back').on('click',function(){
        var $curStep = $nodes.step.filter('.cust-step-cur');
        setCurrent($nodes.list, $curStep, 'cust-step-cur');

        $nodes.stepsSlide.animate({
            right: 0
        }, 0);      
    });
});

Third file PRODUCT-EVENTS.JS

$(document).ready(function(){
    var $productImg = $('.customizer-wrap .image'),
        productImgUrl = $productImg.find('img').attr('src'),
        spinnner,
        $stepsSlide = $nodes.stepsSlide,
        slideCount = $stepsSlide.find('.cust-step').length,
        slidesLength = slideCount * 160;

    $stepsSlide.css({width: slidesLength});

    // Initialize loading graphic
    if(!productImgUrl) {
        return false;
    }

    spinner = startSpinner('product-image', 10, 4, 15, false);

    // Preload the big image, when loaded, fade in
    $.imgpreload(productImgUrl, function(){
        $productImg
            .zoom();

        $('.spinner').fadeOut(2000, function(){
            spinner.stop();
            $('html').removeClass('img-loading');
        });
    });
});
share|improve this question
    
In updateCost(), why do you pass addCost when you set it to 0? –  Larry Battle Sep 3 '12 at 21:16

2 Answers 2

up vote 3 down vote accepted
+25

Here are few things that I noticed with Product.js.

1)

The Module design pattern is normally used to create private functions and variables. Unless if "use strict" is required, then there's no point in wrapping your code inside a closure when all the functions and variables are appended to the global namespace.

2)

Variables shouldn't be passed to a function that won't get used. This is a problem in updateCost() for the variables addCost and totalPrice. This is also a problem for the document variable.

3)

Try to only create varaibles for complex or redundant object references. So name and type aren't needed.

Previous code

var $me = $(value),
    name = $me.attr('data-option'),
    type = $me.attr('data-option-type'),
    option = {
        option: name,
        type: type
    };
product.options.push(option);

New Code


var $me = $(value), option = { option: $me.attr('data-option'), type: $me.attr('data-option-type') }; product.options.push(option);

4)

Instead of extending to the window, just add window. to the desired global variable or function name.

Example: window.$nodes = {};

5)

You should rename getProductOptions() to addOptionsToProduct() since the word get implies a return value.

6)

This is redundant

productPrice = +productPrice;
totalPrice = productPrice + addCost;

Just add productPrice to the end of a numeric operation for it to convert to a number.

totalPrice = addCost + productPrice;

6)

Use existing function instead of writing new ones. For example, take advance of jQuery.each and jQuery.map for interating through collections.

Example: Previous Code

function productOptionsLoop(productOptions, callback) {
    for(var i = 0; i < productOptions.length; i++) {
        callback(i, productOptions[i]);
    }
}
New Code - productOptionsLoop() is the same as the below.
// if jQuery object
productOptions.each( callback );
// else
$.each( productOptions, callback );

7)

Just add the cost, since 0 doesn't affect the value. But it would be even better if you got rid of addCost and used totalCost instead.

productOptionsLoop(productOptions, function(index,value){
    var $me = value,
        cost = $me.cost;
    if(value.cost) {
        addCost += value.cost;
    }
});

Becomes
productOptions.each(function(index,value){
    addCost += value.cost;
});

8)

animateNumber() should call a callback once the animation part is complete. This way you don't need a setTimeout function.

Final code

// Set some general element variables used throughout this script
var $nodes = {
    list: $('.cust-list'),
    steps: $('.steps'),
    step: $('.cust-step'),
    stepsSlide: $('.steps-slide'),
    subtotal: $('.customizer .total .price'),
    allTypes: $('.finish-type a'),
    allColors: $('.finish-color a'),
    options: $('.cust-option'),
    checks: $('.cust-option-checklist a')
};
var Product = function(name) {
    this.name = name;
    this.options = [];
};
// Populate the product object with an array of options
var addProductOptions = function($nodes, product) {
    $nodes.options.each(function(index,value) {
        var $me = $(value),
            option = {
                option: $me.attr('data-option'),
                type: $me.attr('data-option-type')
            };

    product.options.push(option);
});

}; // Change the cost according to the added options / variations var updateCost = function(productOptions, $subtotal, productPrice, $nodes) { var totalPrice = productPrice, currentSubtotal = $subtotal.attr('data-subtotal');

productOptions.each(function(index,value){
    totalPrice += value.cost;
});
animateNumber($nodes.subtotal, currentSubtotal, totalPrice, function(){
    $nodes.subtotal.text(totalPrice).digits();
});
$nodes.subtotal.attr('data-subtotal',totalPrice);

}; var updateOptions = function(productOptions, myOption, myName, myCost, myType) { // Go through the array of options and add the selected color and cost to this option productOptions.each(function(index,$this) { if($this.option !== myOption){ return; } $this.name = myName; $this.cost = Math.floor(myCost); if(myType) { $this.type = myType; } }); };

share|improve this answer

Larry has already mentioned avoiding adding your functions/variables to the global namespace. One way you can do this is to create an object and use it in your extend method like so -

(function (window, document, $) {
    "use strict";

    var myNamespace = myNamespace || {};

    // @param {string} name
    myNamespace.Product = function (name) {
        // do stuff
    };

    // etc
    ...

    $.extend(window, myNamespace);

}(window, document, jQuery));
share|improve this answer

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.