What's the best way to refactor this Rails controller?
- by Robert DiNicolas
I'd like some advice on how to best refactor this controller. The controller builds a page of zones and modules. Page has_many zones, zone has_many modules. So zones are just a cluster of modules wrapped in a container.
The problem I'm having is that some modules may have some specific queries that I don't want executed on every page, so I've had to add conditions. The conditions just test if the module is on the page, if it is the query is executed. One of the problems with this is if I add a hundred special module queries, the controller has to iterate through each one.
I think I would like to see these module condition moved out of the controller as well as all the additional custom actions. I can keep everything in this one controller, but I plan to have many apps using this controller so it could get messy.
class PagesController < ApplicationController
# GET /pages/1
# GET /pages/1.xml
# Show is the main page rendering action, page routes are aliased in routes.rb
def show
#-+-+-+-+-Core Page Queries-+-+-+-+-
@page = Page.find(params[:id])
@zones = @page.zones.find(:all, :order => 'zones.list_order ASC')
@mods = @page.mods.find(:all)
@columns = Page.columns
# restful params to influence page rendering, see routes.rb
@fragment = params[:fragment] # render single module
@cluster = params[:cluster] # render single zone
@head = params[:head] # render html, body and head
#-+-+-+-+-Page Level Json Conversions-+-+-+-+-
@metas = @page.metas ? ActiveSupport::JSON.decode(@page.metas) : nil
@javascripts = @page.javascripts ? ActiveSupport::JSON.decode(@page.javascripts) : nil
#-+-+-+-+-Module Specific Queries-+-+-+-+-
# would like to refactor this process
@mods.each do |mod|
# Reps Module Custom Queries
if mod.name == "reps"
@reps = User.find(:all, :joins => :roles, :conditions => { :roles => { :name => 'rep' } })
end
# Listing-poc Module Custom Queries
if mod.name == "listing-poc"
limit = params[:limit].to_i < 1 ? 10 : params[:limit]
PropertyEntry.update_from_listing(mod.service_url)
@properties = PropertyEntry.all(:limit => limit, :order => "city desc")
end
# Talents-index Module Custom Queries
if mod.name == "talents-index"
@talent = params[:type]
@reps = User.find(:all, :joins => :talents, :conditions => { :talents => { :name => @talent } })
end
end
respond_to do |format|
format.html # show.html.erb
format.xml { render :xml => @page.to_xml( :include => { :zones => { :include => :mods } } ) }
format.json { render :json => @page.to_json }
format.css # show.css.erb, CSS dependency manager template
end
end
# for property listing ajax request
def update_properties
limit = params[:limit].to_i < 1 ? 10 : params[:limit]
offset = params[:offset]
@properties = PropertyEntry.all(:limit => limit, :offset => offset, :order => "city desc")
#render :nothing => true
end
end
So imagine a site with a hundred modules and scores of additional controller actions. I think most would agree that it would be much cleaner if I could move that code out and refactor it to behave more like a configuration.
