重构 重复代码怎么才能更好的消除?

kai209209 · March 18, 2014 · Last by lhy20062008 replied at March 26, 2014 · 9463 hits

新人求思路。现在在做一个项目,做到关于数据还原这一块,写了一个方法,如下

def restore_record record_id, activities
  activity_ids = activities.map(&:id)
  if self.name == "Prize"
    record = self.deleted.find(record_id)
    if activity_ids.include?(record.activity_id)
      record.restore
      return true
    else
      return false
    end
  elsif self.name == "Apply"
    record = self.deleted.friendly.find(record_id)
    if activity_ids.include?(record.activity_id)
      self.restore(record.id, :recursive => true)
      return true
    else
      return false
    end
  elsif self.name == "Activity"
    record = self.deleted.friendly.find(record_id)
    if activities.include?(record)
      self.restore(record.id, :recursive => true)
      return true
    else
      return false
    end
  else
    return false
  end
end

虽然上面的方法能正常使用,但是看上去太臃肿,重复的代码太多了,所以进行一次简单的重构,如下


def restore_record record_id, activities
  if ["Prize", "Apply", "Activity"].include?(self.name)
    self.send "restore_#{self.name.downcase}", record_id, activities
  else
    return false
  end
end

def restore_prize record_id, activities
  activity_ids = activities.map(&:id)
  record = self.deleted.find(record_id)
  if activity_ids.include?(record.activity_id)
    record.restore
    return true
  else
    return false
  end
end

def restore_apply record_id, activities
  activity_ids = activities.map(&:id)
  record = self.deleted.friendly.find(record_id)
  if activity_ids.include?(record.activity_id)
    self.restore(record.id, :recursive => true)
    return true
  else
    return false
  end
end

def restore_activity record_id, activities
  record = self.deleted.friendly.find(record_id)
  if activities.include?(record)
    self.restore(record.id, :recursive => true)
    return true
  else
    return false
  end
end

但是重构后发现自己根本没有能力消除更多的重复的代码,但是想了很久依然没能想到能继续消除重复代码的办法,不知道有没有大神能给予一些建议或者思路,小弟在这里非常感谢!

看一看 ruby 元编程

可以试试分三个子类来写

用几个 lambda 把结构相似的块包起来用

所有的 return false 似乎都没有必要……其实 return 就没必要……

  def restore_record(record_id, activities)
    self.send("restore_#{self.name.downcase}", record_id, activities) if can_restore?
end

  def can_restore?
    ["Prize", "Apply", "Activity"].include?(self.name)
  end

  def restore_prize(record_id, activities)
    activity_ids = activities.map(&:id)
    record = self.deleted.find(record_id)
    record.restore if activity_ids.include?(record.activity_id)
  end

  def restore_apply(record_id, activities)
    activity_ids = activities.map(&:id)
    record = self.deleted.friendly.find(record_id)
    self.restore(record.id, :recursive => true) if activity_ids.include?(record.activity_id)
end

def restore_activity(record_id, activities)
  record = self.deleted.friendly.find(record_id)
  self.restore(record.id, :recursive => true) if activities.include?(record)
end

不要return true或者return false。另外我也没觉得代码有多少重复的地方。我倒觉得这个方法本身返回布尔值很不合理。

restore_record的方法名看,这个方法不应该返回布尔值,只要不依赖返回值,一堆条件语句删了后,应该好些,具体的还要根据实际情况自己重构,没有上下文,基本上是谈不上重构。

另外,推荐你调用方法的时候把括号加上,方法参数也是

看《重构》

感觉改前改后的逻辑都很混乱。这个 method 看似 class method,什么deleted这些都是 class 的逻辑。但又没有 self. 而activities又貌似 instance's reflections。

这样看起来很不清楚,怎么又有 records,又有 activities。楼主还是先搞清楚这些,再写个测试再改吧。

#1 楼 @chaixl 看过 2 遍了,但是现在这是实际的东西,跟书本里面的例子有一些比较大的出入

#2 楼 @paranoyang 这个没必要写多 3 个子类

#4 楼 @Kabie @willmouse 这个方法是在 controller 里面调用的,而 controller 是需要根据返回的布尔值进行下一步操作的。

#6 楼 @Rei 好的,谢谢推荐,我会抽空看看的!

#7 楼 @billy 这个是在 models concerns 下 project_support.rb 的方法,在 model 是通过 extend ProjectSupport 代码将其引入的,所以 restore_record 确实是一个 class method,而 deleted 是 gem paranoia 的 only_deleted 方法,activities 是 controller 里调用 restore_record 方法时所引入的一个值

#3 楼 @pepsin 关于 proc 的太高级了,对于我这种新手来说,不敢用啊

def restore_record record_id, activities
  activity_ids = activities.map(&:id)
  result = false
  if self.name == "Prize"
    record = self.deleted.find(record_id)
    if activity_ids.include?(record.activity_id)
      record.restore
      result = true
    end
  elsif self.name == "Apply" or self.name == "Activity"
    record = self.deleted.friendly.find(record_id)
    if activity_ids.include?(record.activity_id) or activities.include?(record)
      self.restore(record.id, :recursive => true)
      result = true
    end
  end
  return result
end

我简单的帮你精简了下逻辑,不用 lambda 啥的了,你试试

感觉这个是用模版模式。

class Base
   def restore
    if valid?
      do_it
      return true
    else
      return false
    end
  end
end

这个是主干,再写几个子类,就可以了。

12 Floor has deleted
13 Floor has deleted
def find_record(record_id)
  "Prize" == name ? deleted.find(record_id) : deleted.friendly.find(record_id)
end

def has_activity?(record, activities)
  return false unless %w(Prize Apply Activity).include?(name)
  "Activity" == name ? activities.include?(record) : activities.map(&:id).include?(record.id)
end

def restore_it(record)
  "Prize" == name ? record.restore : restore(record.id, :recursive => true)
   true
end

def restore_record record_id, activities
  record = find_record(record_id)
  record.present? && has_activity?(record, activities) && restore_it(record)
end

#10 楼 @pepsin 非常感谢!我试过了,是能用的

#14 楼 @5swords 非常感谢,这代码给我的感觉很惊艳,非常喜欢这个结构,小弟受教了!

一步一步来吧,过度重构也没什么好处。

small_fish__ in 再谈重复代码消除 (跟风帖) mention this topic. 03 Apr 10:57
You need to Sign in before reply, if you don't have an account, please Sign up first.