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

Posted by Robert DiNicolas on Stack Overflow See other posts from Stack Overflow or by Robert DiNicolas
Published on 2009-08-12T23:49:41Z Indexed on 2010/04/16 3:03 UTC
Read the original article Hit count: 238

Filed under:
|
|
|

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.

© Stack Overflow or respective owner

Related posts about ruby-on-rails

Related posts about ruby