如何使用 Rspec 重构 Rails 模型中的复杂方法？答案

【问题标题】：How to refactor complex method in Rails model with Rspec?如何使用 Rspec 重构 Rails 模型中的复杂方法？
【发布时间】：2012-10-30 19:38:43
【问题描述】：

我有以下复杂的方法。我正在尝试寻找并实施可能的改进。现在我将最后一个 if 语句移至 Access 类。

def add_access(access)
   if access.instance_of?(Access)
     up = UserAccess.find(:first, :conditions => ['user_id = ? AND access_id = ?', self.id, access.id])
     if !up && company
       users = company.users.map{|u| u.id unless u.blank?}.compact
       num_p = UserAccess.count(:conditions => ['user_id IN (?) AND access_id = ?', users, access.id])
       if num_p < access.limit
         UserAccess.create(:user => self, :access => access)
       else
         return "You have exceeded the maximum number of alotted permissions"
       end
     end
   end
 end

我还想在重构之前添加规范。我加了第一个。应该怎么像别人？

  describe "#add_permission" do
    before do
      @permission = create(:permission)
      @user = create(:user)
    end

    it "allow create UserPermission" do
      expect {
        @user.add_permission(@permission)
      }.to change {
        UserPermission.count
      }.by(1)
    end
  end

【问题讨论】：

这种方法可能很复杂，因为您的模型关系很复杂。这些模型是什么以及它们为什么/如何相互作用？

标签： ruby-on-rails ruby ruby-on-rails-3 rspec refactoring

【解决方案1】：

我会这样做。

使对 Access 的检查更像是初始断言，如果发生这种情况，则会引发错误。

创建一种新方法来检查现有用户的访问权限 - 这似乎可重用且更具可读性。

那么，公司限制对我来说更像是一种验证，将其移至 UserAccess 类作为自定义验证。

class User

  has_many :accesses, :class_name=>'UserAccess'

  def add_access(access)
    raise "Can only add a Access: #{access.inspect}" unless access.instance_of?(Access)

    if has_access?(access)
      logger.debug("User #{self.inspect} already has the access #{access}")
      return false
    end

    accesses.create(:access => access)
  end

  def has_access?(access)
    accesses.find(:first, :conditions => {:access_id=> access.id})
  end

end

class UserAccess

  validate :below_company_limit

  def below_company_limit
    return true unless company
    company_user_ids = company.users.map{|u| u.id unless u.blank?}.compact
    access_count = UserAccess.count(:conditions => ['user_id IN (?) AND access_id = ?', company_user_ids, access.id])
    access_count < access.limit
  end

end

【讨论】：

我假设有一个与 UserAccess 的关联，或者您可以添加一个。
我还建议将 num_p 重命名为 number_of_permissions 和 users 重命名为 company_users_ids。并且一定要确保您正在测试您的代码，它使这样的重构成为一种令人难以置信的快乐体验！
好点 - 我把它放在一边，这样代码的来源就更清楚了，但你是对的。编辑以反映您的更改。
该方法的示例规范应该是什么样的？你能给我一些提示吗？

【解决方案2】：

你有这个类的单元和/或集成测试吗？在重构之前我会先写一些。

假设您有测试，第一个目标可能是缩短此方法的长度。

这里有一些改进：

将UserAccess.find 调用移至UserAccess 模型并使其成为命名范围。
同样，也移动count 方法。

每次更改后重新测试并继续提取直到干净。每个人对干净都有不同的看法，但你看到它就知道了。

【讨论】：

+1 鼓励测试！测试，然后重构，然后测试。冲洗并重复。

【解决方案3】：

其他想法，与移动代码无关，但仍然更简洁：

users = company.users.map{|u| u.id unless u.blank?}.compact
num_p = UserAccess.count(:conditions => ['user_id IN (?) AND access_id = ?', users, access.id])

可以变成：

num_p = UserAccess.where(user_id: company.users, access_id: access.id).count

【讨论】：