新手问题 How to refactor?

xiaoping_rubyist · 2016年02月22日 · 最后由 qinfanpeng 回复于 2016年02月24日 · 2002 次阅读

最近接触到一个前人留下的一个 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

我手上的项目比这个还不如,加油吧

这代码我觉得重新写个更快点.

有测试吗?没有的话,最好先加测试。

看见查询里面有子查询。。。如果关联关系做好了,子查询这种应该不会出现的吧

查询的方法放在 model 里,修改自身的方法放在 model 里 在一个 model 中,不能有修改其他 model 的方法,如果有的话,放在一个独立的 service 里(service 就是个普通 object,存放操作的代码)controller 里调用 service 或 model 的方法,将返回值处理成 view 需要的代码

Rule #0, write tests before refactoring.

#7 楼 @xiaoping_rubyist 还有个建议是,如果代码比较复杂不好写单元测试的话,可以先写功能测试,保证功能正确;重构的过程中,再慢慢加单元测试,最终会出现测试金字塔 的。

需要 登录 后方可回复, 如果你还没有账号请点击这里 注册