Notes

gistfile1.md

#Knockout-Cart notes:

Overall your code looks just fine to me. I did have a number of comments/suggestions. Mostly things that, in my opinion, make the code a bit more idiomatic and help keep the view clean/simple.

##Where to start

It looks like the cart is functioning properly. To get a good idea of how the view model works, it might be a good idea to start with a look at the template that is using your view model.

##Currency formatting - clean up bindings

First thing that jumps out to me is the currency formatting in the bindings.

<td data-bind='text: $parent.toMoney(price * quantity)' style='text-align:right'></td>
<td data-bind='text: $parent.toMoney(discount)' style='text-align:right'></td>
<td data-bind='text: $parent.toMoney((price * quantity) - discount)' style='text-align:right'></td>

This works fine, but is kind of an eye sore and is the type of code that many point to as what is "bad" about Knockout. There are a couple of reusable ways that we could clean this up. One would be to add properies on our view model that contain formatted versions of our numbers that we could bind directly against. There are some nice, reusable ways to do that, especially when dealing with observables. However, in this case this formatting does not need any knowledge of our view model and is really just a concern of the view. This would be a good candidate for a custom binding. A good type of binding to start with is one that simply "wraps" an existing binding. We like what the text binding does, but just want it to format as currency before updating the text.

//custom binding for currency formatting (wrapper to text binding)
ko.bindingHandlers.currency = {
    update: function(element, valueAccessor) {
        //unwrap the amount (could be observable or not)
        var amount = parseFloat(ko.utils.unwrapObservable(valueAccessor())) || 0;

        //could set the innerText/textContent directly or use $.text(), but we will just let the real text binding handle it by passing it our formatted text
        var newValueAccessor = function() {
            return "$" + amount.toFixed(2);
        };

        //call real text binding
        ko.bindingHandlers.text.update(element,  newValueAccessor);       
    }        
};

We know that the text binding has been thoroughly tested and works across browers, so I like to just call into it.

Now, we can replace the bindings with:

<td data-bind='currency: price * quantity' style='text-align:right'></td>
<td data-bind='currency: discount' style='text-align:right'></td>
<td data-bind='currency: (price * quantity) - discount' style='text-align:right'></td>

There is still the concern that we are doing some simple math in the binding string. Probably better to create discrete values to bind against on the view model. The binding is cleaner, it is easier to use the discrete values, and if the definition changes, then the binding stays the same. Probably can fix this as we consider what should and should not be observable on the model.

##Reacting to quantity changes

The next thing that stands out to me is the extra change handler that is attached to the quantity fields.

<input class='qty' data-bind='value : quantity, event:{change:$parent.quantityChanged}' style='width:24px' type='text'>

If it ever feels like we need to poke Knockout to get it to re-render or re-evaluate changes, then we are probably not taking full advantage of Knockout's strengths. Generally, we want the view model to be robust enough to keep itself in the right state no matter if a value was changed in the UI or programmatically. This is a case where the template is complicated by adding this extra binding and programmatic changes to the quantity would also need to make this call.

In looking at the logic that happens when a single item's quantity is changed, we find that:

• if the quantity is bad, it gets set to 1
• if the quantity is 0, we call self.refreshItems which removes all of the items and adds them back to force a re-render
• we also have a subscription to items that recalculates all of the discounts and saves the array to local storage
• the subscription is triggered when a 0 quantity item is removed, when we remove all of the items, when we add them all back, and when we add a new item. Doesn't seem like changing the quantity on one item should be doing so much work.
• all of this causes the whole cart to be removed and re-rendered on a single change

It feels like the cart is responsible for too much and we are doing a lot of recalculation and trickery to get Knockout to keep itself updated. Definite red flag that we are not fully taking advantage of Knockout. Better to push much of this logic into an individual CartItem.

We need a number of changes to get this into a better state.

##quantity should be an observable

In Knockout, we really want to use observables for anything that can change where we need the UI to be notified or we want to be notified programmatically (either through a manual subsciption or a computed observable).

First thing is to make quantity an observable. We need the UI to update several things when it changes and when the quantity is 0 we need to remove it from the observableArray.

###Forcing quantity to be numeric

When a user enters a value it is a string. In our quantity observable, we really always want to be dealing with an integer. Otherwise, we have to have parseInt calls all over the place.

One option would be to create a manual subcription to quantity and fix it up whenever it changes. That generally works, but it allows the value to get set initially as a string (something could react to it) and when the values is fixed it calls the subscription again. Probably not a performance issue, but with bad logic you could hit a infinite, recursive loop.

Another option is to create a writeable computed observable to bind the UI against. A writable computed allows us to intercept writes and perform logic before setting some underlying observable. First we can do this manually in your view model and then we can talk about how to do this in a more reusable way.

  cartItem.quantity = ko.observable(qty).forcePositiveInteger(1);
  cartItem.quantityForEditing = ko.computed({
      read: cartItem.quantity,
      write: function(newValue) {
         var parsed = parseInt(newValue, 10);
         if (!isNaN(parsed) || parsed < 0) {
             parsed = 1;
         }

         cartItem.quantity(1);
      }
  });

Now we can bind our field against quantityForEditing. However, this adds some bloat to our view model and is not very reusable. There are a couple of ways that we can make this easily reusable.

The first is add an extension to observables to hook this up. Knockout has two ways of extending the core structures. One is called extenders and the other is by augmenting the fn object (which serves as the "prototype") of the base types. I prefer the latter, as it lets you better target specific types (observable vs. computed vs. observableArray) and generally results in cleaner code.

It might look like:

//extension to replace an observable with a writeable computed that forces write to be numeric
ko.observable.fn.asPositiveInteger = function(defaultForBadValue) {
  var original = this,
      interceptor = ko.computed({
        read: original,
        write: function(newValue) {
            var parsed = parseInt(newValue, 10);
            //if value is bad or negative, then use default
            if (isNaN(parsed) || parsed < 0) {
                 parsed = defaultForBadValue
            } 
            original(parsed);
        }
     });
  
     //process the initial value
     interceptor(original());
  
  //return this new writeable computed to "stand in front of" our original observable
  return interceptor;
};

Now, we can define quantity like:

cartItem.quantity = ko.observable(qty).asPositiveInteger(1);

Now, quantity is actually a writable computed that is hiding an actual observable. However, nobody using quantity even needs to know about it.

I generally like an approach like this best, because the logic is in the view model and no matter how quantity is set (in UI or programmatically), we will make sure that it is a positive int. It is also possible to push this into the UI, where we create the writeable computed in a binding and the call the real value binding on this new computed. Would look like:

//wrapper to value binding that forces string to numbers
ko.bindingHandlers.positiveIntegerValue = {
    init: function(element, valueAccessor, allBindingsAccessor, vm, context) {
        var original = valueAccessor();
        var interceptor = ko.computed({
            read: original,
            write: function(newValue) {
                var parsed = parseInt(newValue, 10);
                //if value is bad or negative, then use default
                if (isNaN(parsed) || parsed < 0) {
                     parsed = 1
                } 
                original(parsed);
            }
        });

        //replace our observable with the interceptor
        var newValueAccessor = function() {
            return interceptor;
        };

        //call real value binding's init with our interceptor
        ko.bindingHandlers.value.init(element, newValueAccessor, allBindingsAccessor, vm, context);
    },
    update: ko.bindingHandlers.value.update //call real value binding's update
};

Just depends on how you want to approach it. Is it the view model's or view's job. Generally more robust to make sure that the view model can always handle it.

##Discount is always calculated, so it can be a computed When quantity changes, we need to update the discount value. Since, the discount is always calculated and now that quantity is an observable, we can simply make it a computed.

While we are at it, we can also add some computeds for the amounts that we want to bind against in the UI like:

  //computed to easily bind against for our line's price before discount
  cartItem.linePrice = ko.computed(function() {
    return cartItem.price * cartItem.quantity();      
  });

  //discount is always calculated, so let's use a computed for it
  cartItem.discount = ko.computed(function() {
      return cartItem.quantity() >= 5 ? cartItem.linePrice() * 0.2 : 0;
  });
  
  //computed to easily bind against for the actual total
  cartItem.lineTotal = ko.computed(function() {
    return cartItem.linePrice() - cartItem.discount();
  });

Now, our bindings will simplify down to:

<td data-bind='currency: linePrice' style='text-align:right'></td>
<td data-bind='currency: discount' style='text-align:right'></td>
<td data-bind='currency: lineTotal' style='text-align:right'></td>

##Handling quantity changing to 0 Whenever the quantity is changed to 0, we want to remove the item from the cart. Now that quantity is observable, we should be able to orchestrate that in our view model.

We know that you can create a manual subscription to an observable and do something, but when we do that we are going to need access to the cart item and the cart itself.

So, the choices are:

• we have the cart manage subscribing to every cart item's quantity
• or we give the cart item access to the cart, so that it can remove itself.
• move to something like a pubsub system where an item signals that it has 0 items and the cart removes it (overkill for this)

Subscribing to the quantity every time that we create an item seems a little repetitive, but requiring that a child knows about its parent can also lead to problems. Not that big of a deal in this case, but it would be nice if a cart item could stand on its own. Either would get the job done.

I think that a compromise would be to allow a cart item to receive a function to handle when its quantity changes. Just an easier way to write this up.

Something like:

Tekpub.CartItem = function(options, quantityChangedCallback){
  var cartItem = {};
   
   ...

  //run parent's callback, if specified. Also, if value is not a number, then set value to 1
  if (typeof quantityChangedCallback === "function") {
    cartItem.quantity.subscribe(function(quantity) {
         quantityChangedCallback.call(cartItem, cartItem);   
    });        
  }

Really you could just put quantityChangedCallback(cartItem);. That just always makes me nervous as this will be the window even though it doesn't matter in this case, as this will not be used in your callback.

We also need a function on the view model, something like:

//remove an item if quantity is 0, passed to CartItem self.itemQuantityCheck = function(item) { if (item && item.quantity() === 0) { self.items.remove(item); } };

###Creating Tekpub.CartItem objects Now that our cart item's have observables and other things going on, we need to make sure that we are creating them properly. This needs to be done when we pull them out of local storage and when adding a new item.

Something like:

  var stored = JSON.parse(localStorage.getItem("tekpubCart")) || [];  
  
  self.items = ko.observableArray();
  
  //remove an item if quantity is 0, passed to CartItem
  self.itemQuantityCheck = function(item) {
      if (item && item.quantity() === 0) {
          self.items.remove(item);
      }
  };
 
  //send the items that we load from storage through the CartItem constructor
  self.items(ko.utils.arrayMap(stored, function(item) {
     return Tekpub.CartItem(item, self.itemQuantityCheck);
  }));

and AddItem can look like:

  self.addItem = function(item){
    var existing = self.find(item.sku);
    var items = self.items();

    if (existing) {
      existing.quantity(existing.quantity() + parseInt(item.quantity || 1, 10));
    } else {
      existing = Tekpub.CartItem(item, self.itemQuantityCheck);
      self.items.push(existing); 
    }
    
    return existing;
  };

###Saving to local storage When we save to local storage, we are now dealing with observables in our cart item. We would need to use ko.toJSON to ensure that the observable's inside of the items are unwrapped.

localStorage.setItem("tekpubCart", ko.toJSON(self.items)); 

###Dirty tracking Previously, you had a subscription to items that was writing back to local storage. Now, we can change the quantity on an individual item without removing/adding all of the items. So, we need to make sure that this is getting triggered, whenever any of these things change (individual item's quantity or adding/removing item).

There is a nice way to use a ko.computed to create a dirty flag. We know that a computed is triggered whenever any of its dependencies change and we know that ko.toJSON recursively goes through a structure and unwraps all of its observables. A computed does not even have to return a value. We could do:

//dirty tracker ko.computed(function() { localStorage.setItem("tekpubCart", ko.toJSON(self.items)); });

Now, whenever anything changes that is observable, we will write back to local storage. However, if you were to put a console.log in there, then you would find that it is getting triggered a lot. This is because changing quantity also triggers a change to discount.

Knockout has an extender to "throttle" observbales and computeds. When used on a computed, this means that it will not recompute itself until it stops being notified of changes for x milliseconds.

We can use it like:

//dirty tracker ko.computed(function() { localStorage.setItem("tekpubCart", ko.toJSON(self.items)); }).extend({ throttle: 1 }); //throttle so batches of changes only save once

This will allow all of the changes in the current execution finish before recalculating the computed. Somewhat similar to doing a setTimeout(myFunction, 0);

Now, the item's handle keeping themselves in sync, we have nice things to bind against, and we do not need to trick Knockout into making updates.

##Making it a plugin Based on the README and looking at the code, it seems like it is more of a reference sample, than a reusable library/plugin. I think that there are ways that it could become more of a plugin, if you wanted to go that direction.

A few ideas:

• For starters, I would remove the ko.applyBindings call, as it would cause a conflict with other Knockout code on the page. Someone already using KO might be someone interested in your cart, so this would be a problem. For those that don't use KO, then maybe provide an init function that adds a template binding to an element, then calls ko.applyBindings. The user would not need to know anything about KO (well they would need to update the template).
• To make it easier, you could even include a custom binding like tekpubCart that wraps the template binding. The dev would be able to bind a Tekpub.Cart instance against it. Here is a sample. Otherwise they could just use the template binding themselves. For non-KO users, you could add the init function to add the binding and call ko.applyBindings.
• Sometimes a custom binding even includes the default template as a string in the binding. For your case, I am not sure that you can create a template that is really generic/flexible enough that someone would not need to customize it, so I wouldn't go any further in that direction. If you did, then the Cart object would likely need other properties to describe the title, URL, and column headers, etc.

##Object creation patterns

Not a big deal, but as far as patterns go your CartItem returns a new object each time (don't need to call it with new and doesn't rely on "this" being a new instance), while your Cart follows a more traditional constructor approach. This doesn't really cause any issues other than an inconsistency and you don't have to have "new" when calling Tekpub.CartItem (it is not a real constructor, although it looks like one). Luckily, calling it with new anyways, doesn't cause an issue.

I am not saying that I would necessarily change it, just pointing out the inconsistency. Doesn't seem necessary (or give an advantage) to be inconsistent there.

##Discount double-check

In the template, you are showing the line total using this formula: (price * quantity) - discount. On your CartItem objects you have a lineTotal function, that uses this logic: (cartItem.price - cartItem.discount) * cartItem.quantity. The discount updating logic would have already taken the quantity into account, so lineTotal does not seem to have the correct logic.

Additionally, there is a priceInPennies that doesn't seem to be used anywhere other than in a test. If you need it, not sure why it is a function rather than a computed (easier to bind against, no parens needed when not in an expression).

##Contains function The contains function on the Cart is not working properly. When you do self.items.indexOf it expects you to pass the specific object reference that you are lookling for. In your test it returns indexOf returns -1, which is actually considered truthy in the assertion (.toBeTruthy). Probably you can just use your find function instead. The logic for contains would really be the same, except you would possibly force it to be a boolean to make it a nice API. Basically:

self.contains = function(sku) {
   return !!self.find(sku); //force to boolean
};

I would probably just remove it.

##jQuery dependency Currently, cart.js does use jQuery in two places. Seems like this dependency could be removed, as KO doesn't require jQuery itself (of course jQuery is a default for the majority of devs). Based on the section about not calling ko.applyBindings, seems like the ready function could be removed. The other place is reading the data- attributes using data() in the addClicked function. That could be replaced with plain getAttribute calls.

Otherwise, you should reference jQuery in SpecRunner.html, as it causes an error.

##addClicked handler Normally, it is a bit of a red flag to see a handler accessing the event object and especially looking at DOM elements in the view model. We had that discussion one time in email, although not sure that I convinced you sufficiently. Ideally you want to avoid DOM references in your view model. It should be able to stand alone wthout a view.

I can understand in your case that addClicked is just a wrapper to addItem that first pulls the value out of data- attributes. Seems reasonable and realistic. The data- attributes allows someone not using KO to use the cart pretty easily.

Not something that would be necessary, but I think that it would be interesting to use a custom binding to pull the data- attributes prior to calling the handler. Here is a simple handler (using jQuery to grab data):

//wrapper to click binding.  call handler with object from data- attributes
ko.bindingHandlers.clickWithData = {
    init: function(element, valueAccessor, allBindings, vm, context) {
        var data = $(element).data(),
            boundHandler = valueAccessor().bind(this, data); //this is context, data is first argument
        
        delete data.bind; //remove the data-bind attribute's data
        
        //call the real click binding
        ko.applyBindingsToNode(element, { click: boundHandler });
    }        
};

Now, the view model code doesn't need to know about DOM elements or data- attributes.

Sample here.

##Checkout click handler

Since your "Save & Checkout" button is already a submit type, you should not need the click handler that you attach at the end of the file in jQuery. A submit button will submit the form when you click on it or hit enter in the form.

##Template

The last four elements look like they are extra:

</aside>
</div>
</div>
</div>

##Update Sample Here is a version of it that I have updated with most of these changes and a few more: http://jsfiddle.net/rniemeyer/3dJAz/

A few more changes in it:

• made a computed (was just a function) for total on the Cart and used lineTotal for each item.
• consolodated removeClick and remove logic. Didn't seem like you needed two methods there.

I would be more than happy to explain/discuss/elaborate on any of these suggestions.

Thanks- Ryan

I'm experimenting with Knockout.js, and this helped me understand how to stop my HTML from looking like 1990s-era onclick spaghetti. Thanks!

Love the feedback. But I noticed have a subtle display logic bug with the asPositiveInteger extenstion. If you convert the "bad" value twice in a row then the value of the target doesn't change and dependencies are not notified.

In your jsFiddle, add an item to the card, then change the quantity to -1. It is corrected to 1 as expected, but it you do it again the text box stays at -1, even through the underlying observable's value is 1

My write functions check the current value and manually trigger a subscription update if required. The if/else block is required due to the way the dependency checking works.

    write: function (value) {
        var parsed = parseInt(value, 10);
        var manualNotifyFlag = false;
        if (isNaN(parsed)) {
            parsed = defaultValue;
            manualNotifyFlag = ( original() === parsed );
        }

        if (!manualNotifyFlag) {
            original(parsed);
        } else {
            original.valueHasMutated();
        }
    }