最近接触到一个前人留下的一个 rails 项目。controller 里面内容太多，而 model 里面几乎没内容。想正好边学边重构下。 下面这个 controller 里面的代码太多。比如：

@topologies = Topology.find(:all, :conditions => ["status != ? AND testbed_id IN (?) AND location_id IN (?) AND username ~* ?", 'TERMINATED', Testbed.where(:tb_type => "OSEXEC").all.map {|t| t.tbid}, @locations, @user ])

是不是应该写成函数添加到 model 里面？但是感觉又有很多的查询条件要传入函数里面，几个表都有关联，该怎么分割？学了小半年 rails 了。还是不怎么熟悉。求指导。而且这个项目写的好简陋，语法什么感觉都不规范，想改造下，目前有觉得能力不够。看 ruby-china 的源代码和这个感觉完全是两个世界。 controller

class TopologiesController < ApplicationController
  before_action :is_user?,  :only => [:index, :test, :visualize, :create, :destroy], :unless => :format_json?
  before_action :is_admin?, :only => [:new, :edit, :update]
  before_action :validate_token, :only => [:create, :destroy, :reboot ]
  def format_json?
    if request.format == 'application/json' then
      return true
    else
      return false
    end
  end
def index 
    @user = get_user
    @role = get_user_role
    @locations = Location.all.map { |l| l[:locationid] }
    params[:location] = session[:location] if !session[:location].nil?
    if !params[:location].nil? then
      @locations = [Location.where({:name => OSUtils.get_location_name(params[:location])}).first.locationid]
    end

    # Filter based on status
    if @role == 'Admin' then
      if params[:status] then
        @topologies = Topology.where({:status => params[:status], :location_id => @locations}).all
      elsif params[:tb_type] && params[:tb_type] == "setup" then
        @topologies = Topology.find(:all, :conditions => ["status != ? AND testbed_id IN (?) AND location_id IN (?)", 'TERMINATED', Testbed.where(:tb_type => "OSSETUP").all.map {|t| t.tbid}, @locations])
      else
        @topologies = Topology.find(:all, :conditions => ["status != ? AND testbed_id IN (?) AND location_id IN (?)", 'TERMINATED', Testbed.where(:tb_type => "OSEXEC").all.map {|t| t.tbid}, @locations ])
      end
    else
      @topologies = Topology.find(:all, :conditions => ["status != ? AND testbed_id IN (?) AND location_id IN (?) AND username ~* ?", 'TERMINATED', Testbed.where(:tb_type => "OSEXEC").all.map {|t| t.tbid}, @locations, @user ])
    end

    # Add testbed name to the records
    @topologies.each do |topology|
      topology["testbed_name"] = Testbed.find(topology.testbed_id).name
    end
    @topologies.sort_by! { |t| t["testbed_name"] }

    respond_to do |format|
      format.html # index.html.erb
      format.json { render json: @topologies }
    end
  end
end

model

class Topology < ActiveRecord::Base
  attr_accessible :hold_time, :location_id, :status, :testbed_id, :topology_definition_resolved, :topology_definition_unresolved, :username, :created_by, :create_at
  validates :status, inclusion: { in: [ "IDLE", "INUSE", "TERMINATED" ] }
end

class Testbed < ActiveRecord::Base
  attr_accessible :alias, :feature_group_id, :location_id, :name, :openstack_status, :platform_group_id, :qbs_server_id, :remark, :status_id, :tb_config, :tb_diagram, :user_group_id, :tb_type, :locked_by

  self.table_name = 'testbed'
  alias_attribute :testbed_id, :tbid
  alias_attribute :qbs_server_id, :qbs_serverid
  alias_attribute :location_id, :locationid
  alias_attribute :user_group_id, :user_groupid
  alias_attribute :status_id, :statusid
  alias_attribute :feature_group_id, :feature_groupid
  alias_attribute :platform_group_id, :platform_groupid

  validates_uniqueness_of :name

  has_many :hosts, :foreign_key => :tbid
end

routes

resources :topologies