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.

As part of a larger web app we are producing, I was asked to help build functionality to tie a page's shopping cart into localStorage for persistence. What follows are the three functions that I wrote that produce a JSON representation of the cart, and stores it in LS.

As far as I can tell, it all works great. I'm curious if I'm missing any best practices, or if I've done something that could prove problematic at scale, etc. Maybe there are just some general improvements I could make or something you would do differently. I just want to make sure I'm always working towards producing the best code I can, and unfortunately I don't have the resources on my team to do internal code review.

Note that all the console logging is mostly there for debugging, and almost all of it will be removed before the code itself goes live. I'm particularly interested if my for-loop approach is the best possible solution for searching and updating existing JSON items.

var cartItems;
var MyApp = {};

// On load, look for an existing cart to preload
MyApp.getCart = function() {
    // Check for localStorage
    if(localStorage) {
          if(localStorage['myapp.cart'] != null) {
              // Get the cart
            cartItems = JSON.parse(localStorage['myapp.cart']);
            checkoutCounter = cartItems.count;
            // Update the button counter value
            counterSpan.text(checkoutCounter);
            // External function to enable the button
            MyApp.theCounter();
        }
    } else {
        console.log('localStorage not detected, cannot get cart.');
    }
};

// Add an item to the localStorage cart
MyApp.saveToCart = function(role) {
    if(localStorage) {
        console.log('localStorage detected. Processing cart item: '+role);
        // Create a new cart array object if there isn't one
        if(cartItems == null) {
            console.log('Cart is empty, preparing new cart array');
            cartItems = {
                'count': '1',
                'items':[{'item':role,'quantity':'1'}]
            };
            console.log('Cart array created');
          } else {
            console.log('Existing cart detected, searching for existing role.');
            var newItem = true;
            // Loop our array to see if we need to update an existing item
            for(var i = 0; i < cartItems.items.length; i++) {
                if(cartItems.items[i].item === role) {
                    console.log('Existing role detected: '+role);
                    newItem = false;
                    var itemQuantity = parseInt(cartItems.items[i].quantity);
                    console.log('Updating current role quantity: '+itemQuantity);
                    itemQuantity++;
                    console.log('New role quantity: '+itemQuantity);
                    cartItems.items[i].quantity = itemQuantity.toString();
                } 
            }
            // We must not have found an existing item, so add one
            if(newItem) {
                console.log('Role not found. Adding role to cart with quantity 1: '+role);
                cartItems.items.push({'item':role,'quantity':'1'});
            }
            // Update the item count
            var cartCount = cartItems.count;
            console.log('Current cart count: '+cartCount);
            cartCount++;
            console.log('New cart count: '+cartCount);
            cartItems.count = cartCount.toString();
        }
        // Push the prepared data into localStorage
        console.log('Saving cart data...');
        localStorage['myapp.cart'] = JSON.stringify(cartItems);
        console.log('Cart data saved.');
    } else {
        console.log('localStorage not supported, item not saved for later');
    }
};

// Remove an item from the localStorage cart
MyApp.deleteFromCart = function(role) {
    if(localStorage) {
        console.log('localStorage detected. Removing cart item: '+role);
        // Blow away the whole cart object from localStorage if it only held this one item, it's assuming the one item must match what triggered the remove action
        if(parseInt(cartItems.count) == 1) {
            console.log('Only 1 item in cart. Removing cart object entirely.');
            localStorage.removeItem('myapp.cart');
        } else {
            // Update the item count
            var cartCount = cartItems.count;
            console.log('Current cart count: '+cartCount);
            cartCount--;
            console.log('New cart count: '+cartCount);
            cartItems.count = cartCount.toString();
            console.log('Multiple items in cart, searching for role: '+role);
            // Find the item to update it
            for(var i = 0; i < cartItems.items.length; i++) {
                if(cartItems.items[i].item === role) {
                    console.log('Role '+role+' found with quantity: '+cartItems.items[i].quantity);
                    var itemQuantity = parseInt(cartItems.items[i].quantity);
                    // If there was only one, remove it entirely from the cart object
                    if(itemQuantity == 1) {
                        console.log('Removing role from cart.');
                        cartItems.items.splice(i,1);
                    // Just reduce it by one
                    } else {
                        itemQuantity--;
                        console.log('Updating current role quantity: '+itemQuantity);
                        cartItems.items[i].quantity = itemQuantity.toString();
                    }
                } 
            }
            // Push the prepared data into localStorage
            console.log('Saving cart data...');
            localStorage['myapp.cart'] = JSON.stringify(cartItems);
            console.log('Cart data saved.');
        }
    } else {
        console.log('Local Storage not supported, item not saved for later');
    }
};
share|improve this question
add comment

1 Answer 1

up vote 2 down vote accepted

You should write a thin wrapper around localStorage so that you don't have to constantly ask if (localStorage) every time you want to use it. You can also do that test once, and if it doesn't exist, stub it out with a simple {}:

if (!localStorage) {
  console.log("LocaleStorage is not supported, data will not be persisted")

  // Let the program use a stub object to proceed
  localStorage = {};
  localStorage.prototype.removeItem = function(key) {
    this[key] = null;
  }
}

store = {
  read: function(key) {
    if (localStorage[key])
      return JSON.parse(localStorage[key])

    return null;
  },

  write: function(key, value) {
    localStorage[key] = JSON.stringify(value)
  },

  clear: function(key) {
    localStorage.removeItem(key);
  }
}

Now your methods look much simpler:

// On load, look for an existing cart to preload
MyApp.getCart = function() {
  // Get the cart
  cartItems = store.read("myapp.cart")
  checkoutCounter = cartItems.count;
  // Update the button counter value
  counterSpan.text(checkoutCounter);
  // External function to enable the button
  MyApp.theCounter();
};

There's a ton of cleanup to be done by making this object oriented. You could be creating a ShoppingCart class which has save/load/addItem/removeItem/etc, and then creating a global instance called myCart. It would be much, much cleaner than a bunch of global functions.

By which I mean:

ShoppingCart = function(name) {
  this.name = name;
  this.items = store.read(name)
}

ShoppingCart.prototype = {
  save: function () {
    store.write(this.name, this.items)
  },


  addItem: function (role) {
    if (this.items[role])
      this.items[role] += 1;
    else
      this.items[role] = 1;

    this.save()
  },

  removeItem: function(role) {
    if (this.items[role])
      this.items[role] -= 1;

    if (this.items[role] == 0)
      delete this.items[role];

    this.save()
  }

}

var myCart = new ShoppingCart("myapp.cart");
share|improve this answer
    
Thanks for the idea, I like it. I assume in your example above you mean function(key), rather than function(name), right? –  TheQuicksilver Feb 14 '13 at 15:41
    
Yeah, now fixed. –  meagar Feb 14 '13 at 15:45
    
So, when you say that I could clean it up by making it object oriented, is that not what I did by namespacing the functions within MyApp? Or am I combining principles where I shouldn't be? –  TheQuicksilver Feb 14 '13 at 15:52
    
You've got a lose series of functions writing directly to a global variable called cartItems. JavaScript does provide a level of encapsulation that you're not taking advantage of. –  meagar Feb 14 '13 at 16:12
    
@TheQuicksilver See my update for a simplified ShoppingCart class. –  meagar Feb 14 '13 at 16:19
add comment

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.