Task E: Check Out #1

On the question of how to more loosely couple the methods, IMHO, the "Order#add_line_items_from_cart" does too much. The order roughly maps to a cart, while a product roughly maps to a line item. I would pull the iteration through the "cart_items" out of "add_line_items_from_cart" and place it in "from_cart_item". Here's my modified (and renamed) version of that code:

In OrderController.rb:

def save_order
  @cart = find_cart
  @order = Order.new(params[:order])
  # @order.add_line_items_from_cart(@cart)
  @order.build_from_cart(@cart)
 
  if @order.save
    session[:cart] = nil
    redirect_to_index("Thank you for your order")
  else
    render :action => :checkout
  end
end

In order.rb

  def build_from_cart(cart)
    line_items << LineItem.from_cart(cart)
  end

In line_item.rb

  def self.from_cart(cart)
    items = []
 
    cart.items.each do |cart_item|
      li = self.new
      li.product = cart_item.product
      li.quantity = cart_item.quantity
      li.total_price = cart_item.price
      items << li
    end
 
    items
  end

linoj says:

I question if the above solution does very much. It simply moves the loop from Order to LineItem?, and doesnt relieve the coupling, as LineItem? still needs to know how CartItem? is implemented. I think the answer lies in letting from_cart_item take values from cart_item without it having to directly know the details of cart_item. I dont know Ruby that well, but perhaps LineItem? could be made a subclass of CartItem??

carlosk says:

In my opinion the tight coupling that happens is the presence of both the classes LineItem? and CartItem?. I managed to remove the CartItem? class, so now the LineItem? class no longer needs to know anything about the CartItem? class (since there is no CartItem? class). While making the required refactorings I found an issue. My initial attempt for implementing the increment_quantity method was the following:

In line_item.rb

def increment_quantity
  quantity += 1
  total_price = product.price * quantity
end

When I tested the page, I found that the quantity wasn't incrementing. After some trial and error I managed to get this method:

In line_item.rb

def increment_quantity
  self.quantity += 1
  self.total_price = product.price * quantity
end

It seems that adding the "self" keyword managed to make the changes of the quantity value persistent. I really don't understand why this works… Anyone has any ideas???

jeromio says:

I can't address your issue with class member access, but I agree with your architecture. It dovetails with the need for the cart to be in the database, as opposed to the session. The cart _is_ an order, line items are cart items. Check-out merely transfers ownership from the customer to the order handler (the back-end store). That change of ownership does nothing to the underlying structure/schema.

Not sure how you solved the specifics of the back-end fulfillment (it's skipped in the book also). I presume/propose that the table containing cartitems and line_items is the same table (since those are the same objects). The orders table will point to cart ids that are orders. If there is no order associated with a cart, then it's just a cart, not yet an order. Cart/Line items can also be in multiple states: browsing, ordered, wishlist, etc.

Jim Davis says:

A guess (I am a newbie): Assigning to self.quantity forces the assignment to use the quantity= accessor (mutator) method. Without it, it's an assignment to a local variable. I'm guessing that ActiveRecord? creates mutator methods for you by reflecting on the schema.

Anthony Ettinger says:

I think the original problem is that the attributes of cart_item are hardcopied into line_item. I am thinking of a solution similar to CartItem? model, perhaps attr_reader used inside LineItem?, but I'm not sure.

You could probably loop through cart_item and map its properties/attributes to line_item, but again I'm not sure you want all of them should your model for cart_item change. I think the best way is to get your attributes from the line_items table, and then grab those out of the cart_item object if they exist.

Russell Healy says:

I think the bad coupling is between Order and CartItem. Cart owns the CartItems, and should have the responsibility for working with them, as follows:

In store_controller.rb:

def save_order
  @cart = find_cart
  @order = Order.new params[:order]
  @cart.fill @order
  if @order.save
   # ...

In cart.rb:

def fill(order)
  @items.each do |cart_item|
    line_item = LineItem.from_cart_item(cart_item)
    order.add line_item
  end
end

In order.rb:

def add(line_item)
  line_items << line_item
end

Donald Jean-Baptiste says:

At first, I came up with the same solution as the first post, but like Linoj, I didn't feel it was the right solution.

I felt like the CartItem should manage the conversion to a LineItem (kinda like 5.to_s = "5")

Finally I came up with this:

in store_controller.rb:

@cart = find_cart
    @order = Order.new(params[:order])
    @order.add_line_items_from_cart(@cart)
    if @order.save
    ....

In order.rb:

def add_line_items_from_cart(cart)
     cart.items.each do |item|
         line_items << item.to_line_item
     end
end

in cart_item.rb:

def to_line_item
    li = LineItem.new
    li.product  = self.product
    li.quantity = self.quantity
    li.total_price  = self.price
    li
end

I commented out the code in line_item.rb.

The Moose Says

I don't claim to be an expert but I reckon you need to decouple both LineItem and Order classes from the Cart class. Before you make the changes both LineItem and Order could be adversely affected by changes to the Cart class, and what if you wanted to create Orders in the future from sources other than a Cart? My solution would be:

in line_item.rb:

  def self.create_line_item(product, quantity, price)
    li = self.new
    li.product     = product
    li.quantity    = quantity
    li.total_price = price
    li
  end

Note, this is now completely de-couple from the Cart class

in order.rb:

    def add_line_item(line_item)
       line_items << line_item
   end

Again, completely decoupled from Cart class.

in store_controller.rb:

  def save_order
    @cart = find_cart
    @order = Order.new(params[:order])
 
    @cart.items.each do |item|
       li = LineItem.create_line_item(item.product, item.quantity, item.price)
       @order.add_line_item(li)
    end

I was thinking I might split that last bit of code down and pull out the loop into a separate (maybe private) method in the StoreController. Any thoughts?

I personally reckon it would be good if the author of the book added some comments here, considering he sent us here for tips in the first place!!!

Javi Says

I agree with some of The Moose's points. I'd do the same changes to line_item.rb, but I'd chage the Store Controller so it isn't coupled to LineItem class:

def save_order
  @cart = find_cart
  @order = Order.new(params[:order])
  @order.line_items =  @cart.create_line_items
  #...
end

Then I'd change the Cart class. In the book, we ask an order object to add line items from a cart object. My point is: since we want to create line items from a cart, we should ask the cart object to generate the line items, and each cart item to create each line item.

Cart.rb:

def create_line_items
  line_items = []
  @items.each do |item|
     li = item.create_line_item
     line_items << li
  end
  line_items
end

CartItem.rb:

def create_line_item
  LineItem.create(@product, @quantity, price)
end

Gregory Says

Hi,

According to ActiveRecord::Base documentation:
"Active Records accept constructor parameters either in a hash or as a block. The hash method is especially useful when you‘re receiving the data from somewhere else….."

Therefore what I propose to decouple LineItem from CartItem is to make following changes:

1. In CartItem class add the method which returns hash that can be used for LineItem initialization:

cart_item.rb

def line_item_hash
    parameters = {:product=>@product, :quantity=>@quantity, :total_price=>self.price}
end

2. Slightly change add_line_items_from_cart method in Order class. Instead of calling LineItem class method we create new LineItem instance, passing hash parameter described above.

order.rb

def add_line_items_from_cart(cart)
    cart.items.each do |item|
        #  li = LineItem.from_cart_item(item)     -- commented out!
        li = LineItem.new(item.line_item_hash)
        line_items << li
    end
end

3. LineItem class looks now pretty simple:

line_item.rb

class LineItem < ActiveRecord::Base
  belongs_to :product
  belongs_to :order
 
end

Ico Says

Hello,

After exploring your solutions came out with combination of those of the Moose & Gregory

Here it is:

In Store Controler
save_order

  def save_order
    @cart = find_cart
    @order = Order.new(params[:order])
 
    @cart.items.each { |i| @order.order_items << OrderItem.new( :product => i.it, :qty => i.qty, :total => i.price ) }
    if @order.save
        session[:cart] = nil
        redirect_to_index("Thank you for your order" )
    else
        render :action => :checkout
    end
  end

In OrderItem -> LineItem according the book we do not need no additional methods so it looks as in George posting

In Order model

class Order < ActiveRecord::Base
  has_many    :order_items 
  attr_writer :order_items
  PAYMENT_TYPES = [
    # Displayed          stored in db
    [ "Check" ,          "check" ],
    [ "Credit card" ,    "cc"   ],
    [ "Purchase order" , "po" ]
  ]
 
  validates_presence_of :name, :address, :email, :pay_type
  validates_inclusion_of :pay_type, :in => PAYMENT_TYPES.map {|disp, value| value}
end

and we done with decoupling :)

But the actual question is do we really need those Cart & CartItem objects or we could implement the functionality into Order & LineItem itself and just save them on order_save :) ?

regards,
ico

vastus Says

Hi,
Thanks for all these posts, I learned some things:) I thought to do this a slightly different way so thought to put it up.

In store_controller I did

    @cart = find_cart
    @order = Order.new(params[:order])
    @order.add_line_items( @cart.items_to_ahash )
    if @order.save
# ...

In order there is

  def add_line_items( collection )
    collection.each do |item|
      li = LineItem.from_hash_item( item )
      line_items << li
    end
  end

Note the collection is the array so we don't have to know how cart stores items. We just expect them to be in the hash.

line_item isn't much different than before…

  def self.from_hash_item( item )
    return nil if item.nil?
    li = self.new
    li.product    = item[:product]
    li.quantity   = item[:quantity]
    li.total_price = item[:total_price]
    li
  end

cart passes the message to cart_items to get themselves into hashes

  def items_to_ahash
    arr  = Array.new
    @items.each { |item| arr << item.to_hash }
    arr
  end

cart_items get themselves into a hash

  def to_hash
    { 
      :product   => self.product,
      :quantity  => self.quantity,
      :total_price => self.price
    }
  end

not sure if there's any improvement to much on the last few ways. I agree it would be nice to have some direction from a pragmatic master:) maybe they're busy with their gerbils? :P

JamesSalter says

I think the key to figuring out the decoupling here is to understand the ownership relationships of the various objects.

1. The Order itself doesn't need to know about the existance of the cart. They are separate concepts, therefore passing CartItem objects to Order methods is Not The Right Way.

2. An Order contains LineItems, and there's no reason in this app for LineItems to be visible outside of Order. In the MVC model, the controller should speak to the model interface itself, and not mess with its data internally. This is sort of clouded by the fact that Rails models are derived from ActiveRecord which seems to encourage you to hack the internals. .. That said, if we had a view which showed pending orders, it would be convenient for the controller to enumerate the Order and access interal LineItems. But in this particular case, all we ever do is add to orders.

My tentatively offered solution is that Order provides an add_item method with parameters as primitive types. This requires the controller to know about Carts, CartItem, and about the Order interface. This large amount of dependancy is typical for a controller in MVC as its supposed to act as a nexus.

Posters above have mentioned that LineItem and CartItem are the same thing - well they are currently, but imagine you had a situation where LineItems had shipping tracking .. that makes them separate from the concept of a Cart Item, which is representative of something that hasn't even been purchased yet. Yeah, you could still use the same class with shipping stuff set to null .. but they feel like separate concepts to me and therefore deserve separate classes.

If we are going to retain them, that means they shouldn't actually know about each other's existance as they live in completely separate containers - the order and the cart.

Great to see so much oo discussion from a bunch of ruby newbs (incl. myself).

Code (quite similar to ico's except I avoid breaking into the Order internals from the controller)

Heres my save_order:

 def save_order

    ...

    # copy contents of cart into order    
    @cart.items.each do |item|
      @order.add_item(item.product, item.quantity, item.price)
    end

Here is Order::add_item; i use the hash constructor of ActiveRecord to assign fields. No knowledge of Carts or Cart Items is required here, so we could actually throw the Cart system out entirely and the ordering system interface doesn't have to change.

def add_item(product, quantity, total_price) 
    line_items << LineItem.new({ :product => product, 
                                 :quantity => quantity, 
                                 :total_price => total_price })
  end

Because I used the hash constructor, we no longer need any logic in LineItem; its a dumb row class that has no dependancies on anything:

class LineItem < ActiveRecord::Base
  belongs_to :order
  belongs_to :product  
end

Matt Stuart Says

I agree with much of what has been said here but I also think that some of you are missing the point what decoupling is intended to do.

JamesSalter makes a few excellent points about how MVC is supposed to be used and with that in mind we have to approach this in a different 'non hack' manor. I personally come from many high level languages not the least of which is Java. One of the nice things about java is Reflection. For those of you who are unfamiliar with the concept of reflection it basically allows me to dynamically manipulate any object in any way that object may be able to manipulate itself without actually knowing anything about the object. This is a high level view of what reflection entails, what it's used for, and why you use it but it does bring up something that I think is important for decoupling. In order to decouple properly and allow for code changes to flow with the minimal number of changes possible we need to keep things dynamic. The static use and declaration of properties should remain inside the controller, model, and view in which they were originally declared.

**Note: in the following notes I refer to Order and Cart Controllers (and their respective models), for ruby this isn't exactly the most accurate use of terms. In this case consider the Order and Cart models as 'model-less controllers'. Sorry for any confusion.

Let me tie this back to the MVC principle. As James said a controller is a broad jumping point to other controllers and it's own personal model. A controller may call on methods from it's own model and in other controllers and it should have to have little knowledge of the inner workings of that controller and whatever model that controller may work with. Essentially a controller and it's paired model should be looked at as a single entity. We had a request to a controller, that controller does it's thing and hands back a nice neat packet of data. The more generic and dynamic that data packet the better. We can further abstract this principle and apply it directly to our problem here. We have the Store Controller (SC) which interacts with Orders (O) and Carts (C). The O interacts with Line Items (LI) and the C interacts with Cart Items (CI). We can conceptually view an O and it's LIs as a single item, we hand a request to O asking for some pack of data, it puts it together for us and hands it back. Other controllers never have to know anything specific about LIs in this case they merely have to know to ask O and what to expect. This same methodology can be applies to the C and CI grouping.

I hope I haven't lost anyone here (I may have lost myself so please forgive me if this rambles), essentially what I'm saying is to decouple we need to go abstract. Conceptually the store controller deals with Orders and Carts which are separate entities both conceptually and physically. As such the ideal solution is to allow for the Store Controller to handle the interaction between these two distinct parts of our application. To build an order we need to know what's in the cart thus we request the items, ideally in some dynamic / generic format, hand these to the order controller and let the it take it from there. In this way the order object never needs to know about cart objects or cart item objects it merely needs to receive a list of a generically formatted items from which it can build it's own line item list.

What's the purpose of all these buzzwords and talking in circles? By using the Store Controller as the liaison between Carts and Orders neither has to know technical details of the other. By keeping data transfers generic changes to Cart Item flow through the code without having to update anything else. Properties can be added to a Cart Item and because a Line Item receives a generic data packet and uses it all this data will be immediately available in said Line Item.

I hope that wasn't too long winded… Code changes are as such:

class StoreController < ApplicationController
    # ...
 
    def save_order
        @cart = find_cart
 
        @order = Order.new( params[:order] )
        @order.add_line_items( @cart.get_cart_items )
 
        if ( @order.save )
            session[:cart] = nil
            redirect_to_index( "Thank you for your order" )
        else
            render :action => :checkout
        end
    end
 
    # ...
end

You'll notice the ONLY place that @product, @quantity, and total_price are statically referenced is in Cart Item. In this way changes only need to be made in one place for new properties.

Aside from comments on how long and boring this was, I would like some feedback. Thanks!

Kero Sinha Says

Matt Stuart nailed it! Bravo Matt, I have to say I've understood what decoupling means and what is the real reason for this exercise just after reading your post. Explanation like this should be included in the book IMHO.

Jinyoung Says

I agree with Matt Stuart says. However I have a little bit different opinions. Let's look at one example of store controller.

def checkout
  @cart = find_cart
  if @cart.items.empty?
    redirect_to_index("Your cart is empty" )
  else
    @order = Order.new
  end
end

In the method, cart's object export it's internal data structure to outside. I think it breaks the encapsulation rule of OO principles. The store controller doesn't need to know how cart store cart item. Just "cart is empty or not" is only one information for deciding what controller will work. So let's make delegation method to cart class and change store controller class to use it.

class Cart
  # ...
 
  def empty?
    @items.empty?
  end
 
  # ...
end

Come back to Matt Stuart's code. In save_order method controller call cart's get_cart_items method. It returns array of hash that represents cart_item's attribute. We can know it, even thought we don't see inner code of get_cart_items, because we made it! However, suppose that we are new team member for maintaining this system, we probably confuse between cart's items and get_cart_items method at first. And it also enforce order class to know how the order class iterate data structure.

I think we can avoid this situation by switch receiver and parameter and change the way that add item to order.

class CartItem
  # ...
 
  def to_hash
    #this is same with details. I just want to use standard method style name. :-)
    { :product => @product, :quantity => @quantity, total_price => self.price }
  end
 
  # ...
end

Dimitris says

I think that the main issue here is to remove the mapping from line_item to cart_item done in the LineItem class to the CartItem class. In this way, changes to the cart_item need only be made in one file, the cart_item.rb. In this sense my code is identical to Donald Jean-Baptiste's
I agree with Donald Jean -Baptiste and some

Unless otherwise stated, the content of this page is licensed under Creative Commons Attribution-ShareAlike 3.0 License