How to refactor complex method in Rails model with Rspec?

Question

I have the following complex method. I'm trying to find and implement possible improvements. Right now I moved last if statement to 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 like to add also specs before refactoring. I added first one. How should looks 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

It's possible that this method is complex because your model relationships are complex. What are these models and why/how do they interact? — pje
– pje, Commented Oct 30, 2012 at 19:49

Andrew Kuklewicz · Accepted Answer · 2012-10-31 17:18:49Z

2

Here is how I would do it.

Make the check on the Access more like an initial assertion, and raise an error if that happens.

Make a new method to check for an existing user access - that seems reusable, and more readable.

Then, the company limit is more like a validation to me, move this to the UserAccess class as a custom validation.

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

edited Oct 31, 2012 at 17:18

answered Oct 30, 2012 at 20:00

Andrew Kuklewicz

10.7k1 gold badge36 silver badges43 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Andrew Kuklewicz Over a year ago

I am assuming there is an association to UserAccess, or that you can add one.

wpp Over a year ago

I would also suggest renaming num_p to number_of_permissions and users to company_users_ids. And definitely make sure you are testing your code, it makes refactorings like this an incredibly joyful experience!

Andrew Kuklewicz Over a year ago

good point - I left that alone so it was clearer where the code came from, but you are right. edited to reflect your changes.

tomekfranek Over a year ago

How should example spec for that method looks like? Could you give me some hints?

Kenrick Chien · Accepted Answer · 2012-10-30 20:12:56Z

2

Do you have unit and or integration tests for this class? I would write some first before refactoring.

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

Here are some improvements to make:

Move the UserAccess.find call to the UserAccess model and make it a named scope.
Likewise, move the count method as well.

Retest after each change and keep extracting until it's clean. Everyone has a different opinion of clean, but you know it when you see it.

edited Oct 30, 2012 at 20:12

answered Oct 30, 2012 at 19:48

Kenrick Chien

1,0468 silver badges22 bronze badges

1 Comment

thisfeller Over a year ago

+1 for encouraging tests! Test, then refactor, then test. Rinse and repeat.

Anthony Alberto · Accepted Answer · 2012-10-30 19:51:34Z

1

Other thought, not related to moving the code but still 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 become :

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

answered Oct 30, 2012 at 19:51

Anthony Alberto

10.4k3 gold badges36 silver badges39 bronze badges

Collectives™ on Stack Overflow

How to refactor complex method in Rails model with Rspec?

3 Answers 3

4 Comments

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

4 Comments

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related