What's the best way to refactor this Rails controller?

Question

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.

Answer 1

You should check out this gem:

http://github.com/josevalim/inherited_resources/tree/master

It is very elegant, and solves all the problems you have.

Answer 2

I'd move your snippet-specific queries into helper methods and get them out of the controller so that the snippets themselves can execute the query via erb and kept DRY and readable via a helper. So instead of referring to @refs in your module, you can instead refer to find_all_refs or somesuch in a module and have that execute and possibly memoize the response.