How to Refactor Controller Code To Model Code

Question

I've been using rails for a while, and my controller code starts to get out of control (no pun intended.) I've heard you want skinny controllers and fat models, and for me this hasn't come naturally in my rails progression.

I'll post one model of my rails app.

line_item #create

def create
  @cart = current_cart
  #product is built from base_product, after finding associated product
  @base_product_id = params[:line_item][:base_product_id]
  get_product_from_base_product
  @line_item = @cart.line_items.build(
    :product_id => @product_id,
    :order_id => nil,
    :weight => params[:line_item][:weight],
    :quantity => params[:line_item][:quantity]
    )
  ## Does a line item with the same product_id already exist in cart?
  if @line_item.exists_in_collect?(current_cart.line_items)
    #if so, change quantity, check if there's enough stock
      if  current_cart.where_line_item_with(@product_id).update_quantity(@line_item.quantity) == true
        @line_item.destroy
        redirect_to base_products_path
        flash[:success] = "Detected Producted In Cart,  Added #{@line_item.quantity} More to Quantity"
      else 
        redirect_to cart_path
        flash[:failure] = "Cannot Add To Cart, We Only Have #{@line_item.product.stock_qty} In Stock"
      end
  else
    if @line_item.stock_check == true
      if @line_item.save
        respond_to do |format|
        format.html { redirect_to base_products_path, 
          :notice => "(#{@line_item.quantity}) #{@line_item.product.base_product.title} Added to Cart." }
        format.xml  { render :xml => @line_item,
          :status => :created, :location => @line_item }
        end
      else
        format.xml  { render :xml => @line_item.errors,
          :status => :unprocessable_entity }
      end
    else
      redirect_to base_products_path
      if @line_item.product.stock_qty > 0
      flash[:failure] = "Sorry! We Only Have #{@line_item.product.stock_qty} In Stock"
      else
      flash[:failure] = "Sorry! That Item Is Out Stock"
      end
    end 
  end
end

controller methods:

private 
def get_product_from_base_product
  @product_id = Product.where(:base_product_id => @base_product_id).where(:size_id => params[:line_item][:size]).first.id
end

LineItem model

class LineItem < ActiveRecord::Base
  belongs_to :product
  belongs_to :cart
  after_create :set_order_weight#, :set_package_dimentions
  after_update :set_order_weight#, :set_package_dimentions
  #max capactiy here
def have_enough_product?
  if self.product.stock_qty > self.quantity
    return true
  else
    self.quantity = self.quantity_was
    self.save
    return false
  end
end
def over_order_cap?
    if self.quantity > 50
        return true
    end
end
def update_quantity(qty)
  if self.have_enough_product? == true
  self.quantity += qty
  self.save
  end
end

def stock_check
  if self.product.stock_qty > self.quantity == true
    return true
  else
    return false
  end
end
def exists_in_collect?(items)
  items.each do |item|
    return true if self.product_id == item.product_id
  end
  return false
end

As you can see, this is somewhat large controller. Is this normal? All the logic is happening there. A user may already have this item in the cart, this item may not be in stock, this item has a capacity and the user is trying to order more than that. What's more, I

I know this isn't a typical stack question. Nothing is broken, but I just don't feel good about my code.

I don't want to make a 'homework' question, but I'd appreciate suggestions on how to refactor controller code with models. I'd appreciate references and suggestions.

Thanks! update added cart.rb

 class Cart < ActiveRecord::Base
   has_many :line_items#, dependent: :destroy
   has_one :order
     def where_line_item_with(prod_id)
       line_items.where(:product_id => prod_id).first
     end
     def sub_total
       self.line_items.to_a.sum { |item| item.total_price }
     end 
 end

Answer 1

I'll answer not because I'm an expert, but because I just recently went through this myself. For me, the realization came when I began putting my app under a test suite. I was having a really tough time testing my controllers actions because they did so many different things.

Here are a couple of reasons why I think you should move and separate the logic of your controller into model methods:

• Reuse of code. I found that instead of writing the same code 3 or 4 different times in different controllers, I could write it once (and test it once!) in the model and call those methods in my controllers.

• Testing. Rather than testing the same logic in 3 or 4 controller actions, you can test it once in the model spec (easier to test there as well). Not only does it save you time writing tests, but it removes the possibility that you implement things differently across controllers for what should be the same logic.

• Readability. I'll be honest, I didn't read through your entire create action because it wasn't very readable. I had a couple similar actions, and I hated trying to debug them because it was hard to follow what was happening. Moving that logic to the model (and separating it into smaller, more specific methods) allows you to focus on the things a controller should be doing (auth, directing, handling params etc.).

So, how to refactor it? I read something in a SO question (can't find it now) that essentially said: if the logic deals with the way resources are associated, it should be in the model. I think that's a pretty good general rule. I would start there. If you don't have a test suite, I would add that simultaneously.