3
\$\begingroup\$

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');
    }
};
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

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");
\$\endgroup\$
5
  • \$\begingroup\$ Thanks for the idea, I like it. I assume in your example above you mean function(key), rather than function(name), right? \$\endgroup\$ Feb 14, 2013 at 15:41
  • \$\begingroup\$ Yeah, now fixed. \$\endgroup\$
    – user229044
    Feb 14, 2013 at 15:45
  • \$\begingroup\$ 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? \$\endgroup\$ Feb 14, 2013 at 15:52
  • \$\begingroup\$ 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. \$\endgroup\$
    – user229044
    Feb 14, 2013 at 16:12
  • \$\begingroup\$ @TheQuicksilver See my update for a simplified ShoppingCart class. \$\endgroup\$
    – user229044
    Feb 14, 2013 at 16:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.