While you still hear “fat models, skinny controllers” from time to time, people have generally caught on that not all code needs to be in a model or controller. Fat models don’t happen all at once, though. I find that you add a method here and a method there until you realize that one of your models is well over 200 lines and has too many methods and too little cohesion. Here’s how I approach this problem.
1. Look at the private methods
Assuming you factor shared code between public methods into private methods on your model, they are the best place to find related methods. Do you have two or more public methods using the same private method? Those methods could likely become the public interface for their own focused class. Also, if you have one public method using multiple private methods, it’s possible that method is so complicated it could be a whole class on its own.
Let’s say we have two public methods that share a private method like this:
class User < ActiveRecord::Base
# many methods
def subscription_active?
most_recent_subscription.active? if has_subscription?
end
def subscription_expiration_date
most_recent_subscription.expiration_date if has_subscription?
end
private
def has_subscription?
subscriptions.any?
end
def most_recent_subscription
subscriptions.order(expiration_date: :desc) # this should be a named scope on Subscription, but it is here for example purposes
end
# many methods
end
The subscription_active?
and subscription_expiration_date
methods are prime candidates to get refactored into their own class.
2. Create a new class and call it from the old one
In this example I would make a new class like so:
class UserSubscriptions
delegate :subscriptions, to: :@user
def initialize(user)
@user = user
end
def subscription_active?
most_recent_subscription.active? if has_subscription?
end
def subscription_expiration_date
most_recent_subscription.expiration_date if has_subscription?
end
private
def has_subscription?
subscriptions.any?
end
def most_recent_subscription
subscriptions.order(expiration_date: :desc)
end
end
Then I would make the User
model look like this:
class User < ActiveRecord::Base
# many methods
delegate :subscription_active?, :subscription_expiration_date, to: :user_subscriptions
private
def user_subscriptions
UserSubscriptions.new(self)
end
# many methods
end
At this point, make sure your tests all still pass. Then, move the tests for these methods from the User
model to tests for the UserSubscriptions
class. Make sure they still pass.
At this point, depending on the size of your code base and the number of callers of these two methods, you may end up stopping here. You may think you haven’t really improved things since you’ve just moved some code around and didn’t reduce the User
model’s public footprint. You did, however, make an obvious place for future developers to add subscription-related code that isn’t inside the User
model. This is an improvement because it discourages people from making the problems in the User
model worse.
3. Change callers to use the new class
If at all possible, though, your goal should be to delete those delegations from the User
model altogether. You do that by changing every instance of user.subscription_active?
to UserSubscriptions.new(user).subscription_active?
. You do the same with subscription_expiration_date
and then you are well on your way to skinny controllers, skinny models.
I hope this helps you reduce the size of classes that get out of control!