Pragmatic Programmers

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

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