4

私は次の複雑な方法を持っています。私は可能な改善を見つけて実装しようとしています。今、私は最後の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
4

3 に答える 3

2

このクラスのユニットテストや統合テストはありますか？リファクタリングの前に最初にいくつか書きます。

テストがあると仮定すると、最初の目標はこのメソッドの長さを短くすることかもしれません。

行うべきいくつかの改善点は次のとおりです。

  1. UserAccess.find呼び出しをモデルに移動しUserAccess、名前付きスコープにします。
  2. 同様に、countメソッドも移動します。

変更するたびに再テストし、きれいになるまで抽出を続けます。クリーンについては人それぞれ意見が異なりますが、それを見るとわかります。

于 2012-10-30T19:48:12.873 に答える
2

これが私がそれをする方法です。

アクセスのチェックを最初のアサーションのようにし、それが発生した場合はエラーを発生させます。

既存のユーザーアクセスをチェックするための新しいメソッドを作成します。これは再利用可能で、より読みやすいようです。

次に、会社の制限は私にとっては検証のようなものです。これをカスタム検証として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
于 2012-10-30T20:00:58.637 に答える
1

コードの移動とは関係ありませんが、それでもよりクリーンな他の考え：

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
于 2012-10-30T19:51:34.043 に答える