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

artone · 发布于 2016年7月05日 · 最后由 artone 回复于 2016年7月13日 · 1234 次阅读
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
jasl · #1 · 2016年7月05日

比较快的重构写法

@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
chenjau · #2 · 2016年7月05日

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

3469
liwei78 · #3 · 2016年7月05日 1 个赞

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

9096
emanon · #4 · 2016年7月05日

假设 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
chenjau · #5 · 2016年7月05日

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

20690
u1440247613 · #6 · 2016年7月05日

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

7楼 已删除
1107
jasl · #8 · 2016年7月05日 1 个赞

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

14793
bestjane · #9 · 2016年7月05日 3 个赞
@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
duanjs · #10 · 2016年7月06日

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

23529
mizuhashi · #11 · 2016年7月07日

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

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

162
quakewang · #12 · 2016年7月08日 2 个赞

用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
suffering · #13 · 2016年7月09日
@projects = Project.includes(:todos).send(params[:filter] == 'closed' ? :closed : :opened).sorted_by_alias_asc
15999
embbnux · #14 · 2016年7月09日

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

3417
artone · #15 · 2016年7月13日

感谢各位!

3417
artone · #16 · 2016年7月13日

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

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