Does this code belong in the model or controller

Question

To start, I'm a pretty new to Rails

I've created some methods and put them into my model, but it looks messy and just wondered if the code belongs in the model or the controller? What makes my code unique (not one model per controller anyhow) is that I have only one model "Products" but have 3 controllers that interact with it, "Merchants, Categories, Brands". Maybe there is an easier way I have completely overlooked?? I don't really want to split the data up into 3 tables / models with links between.

p.s. This is the first time I have slipped away from the comfort of a Rails book so please go easy on me! Any other general suggestions to my code will be much appreciated.

Product model

class Product < ActiveRecord::Base

  validates :brand, :presence => true

  def product_name
    name.capitalize.html_safe
  end

  def product_description
    description.html_safe
  end

  #Replace '-' with ' ' for nice names
  def brand_name
    brand.capitalize.gsub('-',' ')
  end

  def category_name
    category.capitalize.gsub('-',' ')
  end

  def merchant_name
    merchant.capitalize.gsub('-',' ')
  end

  #Replace ' ' with '-' for urls
  def brand_url
    brand.downcase.gsub(' ','-')
  end

  def category_url
    category.downcase.gsub(' ','-')
  end

  def merchant_url
    merchant.downcase.gsub(' ','-')
  end

end

Merchants Controller

class MerchantsController < ApplicationController

  def index
    @merchants = Product.find(:all, :select => 'DISTINCT merchant')
  end

  def show
    @products = Product.find(:all, :conditions => ['merchant = ?', params[:merchant]])
    @merchant = params[:merchant].capitalize.gsub('-',' ')
  end

end

Merchant view (index)

<h1>Merchant list</h1>

<%= @merchants.count%> merchants found

<% @merchants.each do |merchant| %>

  <p><%= link_to merchant.merchant_name, merchant.merchant_url %></p>

<% end %>

Merchant view (show)

<h1>Products from merchant: <%= @merchant %></h1>

<%= @products.count%> products found

<% @products.each do |product| %>

  <h3><%= product.product_name %></h3>

  <p>
    <img src="<%= product.image %>" align="right" alt="<%= product.product_name %>" />
    <%= product.product_description %>
  </p>

  <p><%= product.price %></p>

  <p>Brand: <%= product.brand_name %></p>

  <p>Category: <%= product.category_name %></p>

  <p>Sub category: <%= product.sub_category %></p>

  <p>Merchant: <%= product.merchant_name %></p>

  <p><a href="<%= product.link %>" target="_blank">More information</a></p>

  <hr />

<% end %>

Answer 1

So your data model does seem to be getting to the point where you might at least want to split merchants out. You can tell this from the select 'DISTINCT merchant' query. If your merchants are user-based input and are saved inside your products table it seems like a good time to move them into their own model so that they are easily searchable and manageable. As you get more merchants and more products it will get harder and harder to perform this query. Once you want to add additional merchant information you'll be in a worse position as well. Just keep in mind Rails was made for easy refactoring. Making this change shouldn't be daunting, it should just be another regular task in your agile development process.

What the above change would also allow you to do is change these lines:

@products = Product.find(:all, :conditions => ['merchant = ?', params[:merchant]])
@merchant = params[:merchant].capitalize.gsub('-',' ')

into:

@merchant = Merchant.find_by_name(params[:name])
@products = @merchant.products

You could then have a capitalize and gsub name with a model function:

@merchant.display_name

The next step would be to DRY up your model code a little bit, for example:

class Product
  def brand_name
    make_name brand
  end

  def category_name
    make_name category
  end

  def merchant_name
    make_name merchant
  end

  private

  def make_name name
    name.capitalize.gsub('-', ' ')
  end
end

You could do something similar to the _url functions as well. If you wanted to venture further you could clean this up using meta-programming as well.

Final Thoughts: make sure you actually want to be calling html_safe on your strings. If they are user based input it's best to let them go through the h function in your views. Do you want users to be able to enter HTML strings as brands, merchants and categories? If so, then leave the html_safe string there, otherwise let the strings be made html_safe in your views.

In general you are on the right path: Skinny Controllers and Views and Fat Models is the way to go. This means put your logic and your heavy-lifting into your Models and let your Controllers and Views be small and simple.

Answer 2

You should probably normalise your database. You need 3 tables instead of one: Products, Merchants and Brands. Your product table will then have references to merchant and brand tables. You can then have separate models (with belongs_to/has_many relationships between them) and separate controllers.

You will still be able to write things like product.merchant.name but some of your code will be simpler.