看一看 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

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

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 非常感谢，这代码给我的感觉很惊艳，非常喜欢这个结构，小弟受教了！

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