重构 只有状态不同的条件可以如何重构?

artone · 发布于 2016年07月05日 · 最后由 artone 回复于 2016年07月13日 · 1499 次阅读
3417

原本的写法没什么问题,只是想讨论有没有其他可能性。

case params[:filter]
when 'opened'
  @projects = Project.includes(:todos).opened.sorted_by_alias_asc
when 'closed'
  @projects = Project.includes(:todos).closed.sorted_by_alias_asc
else
  @projects = Project.includes(:todos).opened.sorted_by_alias_asc
end

我把 code 简化方便阅读,想问问大家会如何重构这段?这里面只有状态 query 不同其他都一致,有许多重复的 code,可以如何调整呢?

谢谢!

共收到 15 条回复
1107

比较快的重构写法

@projects = Project.includes(:todos).sorted_by_alias_asc
@projects = case params[:filter]
            when 'opened'
              @projects.opened
            when 'closed'
              @projects.closed
            else
              @projects.opened
            end
96

controller里面尽量少逻辑, 特别是这种没必要的. 把东西放到model里去, 外面使用类似 xxxxx.filter_by(参数) 这样不就成了. 另外类似这种其实只是匹配而不太逻辑的东西, 用动态方法也是可以的.

3469

也不要放到 model 里,写成一个class,放到 lib 里,model 里只是 CRUD,validates ,relationship 等操作

9096

假设 opened, closed 两个 scope 只是查询 state 字段的值:

state = params[:filter] == 'closed' ? 'closed' : 'opened'
@projects = Project.includes(:todos).where(:state => state).sorted_by_alias_asc

假设用了 state_machine 还可以:

state = params[:filter] == 'closed' ? 'closed' : 'opened'
@projects = Project.includes(:todos).with_state(state).sorted_by_alias_asc

假设你的这两个 scope 的实现很复杂:

class XXController < ApplicationController

  def ooxx
    state = params[:filter] == 'closed' ? 'closed' : 'opened'
    @projects = Project.includes(:todos).with_state(state).sorted_by_alias_asc
  end
end

class Project < ActiveRecord::Base

  scope :opened, -> { 很复杂的实现 }
  scope :closed, -> { 很复杂的实现 }

  def self.with_state(state) #假如因为用了 state_machine, 方法重名了,另起一个名字。
    allowed_states = ['opened', 'closed']
    raise ArgumentError("Not allowed state: #{state}") if !allowed_states.include?(state) #直接把 __send__ 的参数暴露出去很危险,小心人家传个 `destroy_all` 进来
    __send__ state
  end
end

如果 includes(:todos).with_state(state).sorted_by_alias_asc 这一长串 scope 使用超过 1 次,还可以简化:

class XXController < ApplicationController

  def ooxx
    state = params[:filter] == 'closed' ? 'closed' : 'opened'
    @projects = Project.ooxx(state) #
  end
end

class Project < ActiveRecord::Base

  scope :opened, -> { 很复杂的实现 }
  scope :closed, -> { 很复杂的实现 }
  scope :ooxx, -> {|state| includes(:todos).with_state(state).sorted_by_alias_asc} #

  def self.with_state(state)
    allowed_states = ['opened', 'closed']
    raise ArgumentError("Not allowed state: #{state}") if !allowed_states.include?(state)
    __send__ state
  end
end

假如 includes(:todos).with_state(state).sorted_by_alias_asc 这样的 scope 只用了 1 次,我也会像上面这样简化,但是如果有人把那串 scope 留在 controller 里,我也能接受。

96

题主的不就是 crud 么? 不过是带了 scope. 而且简单, 也仅和该 model 联系紧密, 也不涉及到跨 model 操作. 做个类放到 lib 不会过度了点?

20690

我觉得楼主这段代码没什么好重构的。 看回帖的方案, 都是回字的几种写法而已。

7楼 已删除
1107

实用主义啦,要是更复杂的查询构建就考虑上 form model 了

14793
@projects = Project.includes(:todos).sorted_by_alias_asc
@projects =  
      if %w(opened closed).include?(params[:filter])
          @projects.public_send(params[:filter])
      else
          @projects.opened
      end
12662

#9楼 正解,用到了元编程😃

23529

就看这一段没什么可以重构的,代码重构是要看业务来的,什么可以重用就抽出,写成依赖注入,单纯看一段代码讲重构只是茴字的几种写法而已。

另外,实际项目中慎用元编程和各种动态方法,大部分都是弊大于利,9楼的其实是更难懂了。

162

用Hash来替换case when是常见的重构方法:

h = Hash.new(:opened).merge('closed' => :closed, 'foo' => :foo, 'bar' => :bar)
Project.includes(:todos).sorted_by_alias_asc.send(h[params[:filter]])
709
@projects = Project.includes(:todos).send(params[:filter] == 'closed' ? :closed : :opened).sorted_by_alias_asc
15999

推荐send, 觉得send还是比较好看得懂的

3417

感谢各位!

3417

#11楼 @mizuhashi 谢谢提醒,我了解这段意思了!

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