How to refactor a complex method in a Rails model using Rspec?

Question

I have the following complex method. I am trying to find and implement possible improvements. Right now, I have translated the last if statement into an Access class.

 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 

I would also like to add specifications before refactoring. I added the first one. How to look like others?

  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 

Answer 1

This is how I do it.

Make the access check more like the initial statement and raise an error if this happens.

Make a new method for checking existing user access - which seems reusable and more readable.

Then the company limit is more like validation for me, move it to the UserAccess class as a user check.

 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 

Answer 2

Do you have unit tests and integration for this class? First I will write before refactoring.

Assuming you have tests, the first goal might be to reduce the length of this method.

Here are some improvements:

• Move the call to UserAccess.find to the UserAccess model and make it a named scope.
• Similarly, move the count method.

Repeat the test after each change and continue extraction until it clears. Everyone has a different opinion, but you know it when you see it.

Answer 3

Other thoughts not related to moving code, but even cleaner:

 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]) 

Can be:

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