Why Ruby Class Methods Resist Refactoring
One of the most common questions I received following my 7 Patterns to Refactor Fat ActiveRecord Models post was “Why are you using instances where class methods will do?”. I’d like to address that. Simply put:
I prefer object instances to class methods because class methods resist refactoring.
To illustrate, let’s look at an example background job for syncing data to a third-party analytics service:
class SyncToAnalyticsService
ConnectionFailure = Class.new(StandardError)
def self.perform(data)
data = data.symbolize_keys
account = Account.find(data[:account_id])
analytics_client = Analytics::Client.new(CC.config[:analytics_api_key])
account_attributes = {
account_id: account.id,
account_name: account.name,
account_user_count: account.users.count
}
account.users.each do |user|
analytics_client.create_or_update({
id: user.id,
email: user.email,
account_admin: account.administered_by?(user)
}.merge(account_attributes))
end
rescue SocketError => ex
raise ConnectionFailure.new(ex.message)
end
end
The job iterates over each user sending a hash of attributes as an HTTP POST. If a SocketError is raised, it gets wrapped as a SyncToAnalyticsService::ConnectionFailure, which ensures it gets categorized properly in our error tracking system.
The SyncToAnalyticsService.perform method is pretty complex. It certainly has multiple responsibilities. The Single Responsibility Principle (SRP) can be thought of as fractal, applying at a finer grained level of detail to all of applications, modules, classes and methods. SyncToAnalyticsService.perform is not a Composed Method because not all of the operations of the method are at the same level of abstraction.
One solution would be to apply Extract Method a few times. You’d end up with something like this:
class SyncToAnalyticsService
ConnectionFailure = Class.new(StandardError)
def self.perform(data)
data = data.symbolize_keys
account = Account.find(data[:account_id])
analytics_client = Analytics::Client.new(CC.config[:analytics_api_key])
sync_users(analytics_client, account)
end
def self.sync_users(analytics_client, account)
account_attributes = account_attributes(account)
account.users.each do |user|
sync_user(analytics_client, account_attributes, user)
end
end
def self.sync_user(analytics_client, account_attributes, user)
create_or_update_user(analytics_client, account_attributes, user)
rescue SocketError => ex
raise ConnectionFailure.new(ex.message)
end
def self.create_or_update_user(analytics_client, account_attributes, user)
attributes = user_attributes(user, account).merge(account_attributes)
analytics_client.create_or_update(attributes)
end
def self.user_attributes(user, account)
{
id: user.id,
email: user.email,
account_admin: account.administered_by?(user)
}
end
def self.account_attributes(account)
{
account_id: account.id,
account_name: account.name,
account_user_count: account.users.count
}
end
end
This is a little better the original, but doesn’t feel right. It’s certainly not object oriented. It feels like a weird hybrid of procedural and functional programming, stuck in an object-based world. Additionally, you can’t easily declare the extracted methods private because they are at the class level. (You’d have to switch to an ugly class << self form.)
If I came across the original SyncToAnalyticsService implementation, I wouldn’t be eager to refactor only to end up with the above result. Instead, suppose we started with this:
class SyncToAnalyticsService
ConnectionFailure = Class.new(StandardError)
def self.perform(data)
new(data).perform
end
def initialize(data)
@data = data.symbolize_keys
end
def perform
account = Account.find(@data[:account_id])
analytics_client = Analytics::Client.new(CC.config[:analytics_api_key])
account_attributes = {
account_id: account.id,
account_name: account.name,
account_user_count: account.users.count
}
account.users.each do |user|
analytics_client.create_or_update({
id: user.id,
email: user.email,
account_admin: account.administered_by?(user)
}.merge(account_attributes))
end
rescue SocketError => ex
raise ConnectionFailure.new(ex.message)
end
end
Almost identical, but this time the functionality is in an instance method rather than a class method. Again, applying Extract Method we might end up with something like this:
class SyncToAnalyticsService
ConnectionFailure = Class.new(StandardError)
def self.perform(data)
new(data).perform
end
def initialize(data)
@data = data.symbolize_keys
end
def perform
account.users.each do |user|
create_or_update_user(user)
end
rescue SocketError => ex
raise ConnectionFailure.new(ex.message)
end
private
def create_or_update_user(user)
attributes = user_attributes(user).merge(account_attributes)
analytics_client.create_or_update(attributes)
end
def user_attributes(user)
{
id: user.id,
email: user.email,
account_admin: account.administered_by?(user)
}
end
def account_attributes
@account_attributes ||= {
account_id: account.id,
account_name: account.name,
account_user_count: account.users.count
}
end
def analytics_client
@analytics_client ||= Analytics::Client.new(CC.config[:analytics_api_key])
end
def account
@account ||= Account.find(@data[:account_id])
end
end
Instead of adding class methods that have to pass around intermediate variables to get work done, we have methods like #account_attributes which memoize their results. I love this technique. When breaking down a method, extracting intermediate variables as memoized accessors is one of my favorite refactorings. The class didn’t start with any state, but as it was decomposed it was helpful to add some.
The result feels much cleaner to me. Refactoring to this feels like a clear win. There is now state and logic encapsulated together in an object. It’s easier to test because you can separate the creation of the object (Given) from the invokation of the action (When). And we’re not passing variables like the Account and Analytics::Client around everywhere.
Also, every piece of code that uses this logic won’t be coupled to the (global) class name. You can’t easily swap in new a class, but you can easily swap in a new instance. This encourages building up additional behavior with composition, rather than needing to re-open and expand classes for every change.
Refactoring note: I’d probably leave this class implemented as the last source listing shows. However, if the logic becomes more complex, this job is begging for a class to sync a single user to be split out.
So how does this relate to the premise of this article? I’m unlikely to see the opportunities for refactoring a class method because decomposing them produces ugly code. Starting with the instance form makes your refactoring options clear, and reduces friction to taking action on them. I’ve observed this effect many times in my own coding and also anecdotally across many Ruby teams over the years.
Objections
YAGNI (You Aren’t Going To Need It)
YAGNI is an important principle, but misapplied in this case. If you open these classes in your editor, neither form is more or less complicated than the other. Saying “YAGNI an object here” is sort of like saying “YAGNI to indent with two spaces instead of a single tab.” The variants are only different stylistically. Applying YAGNI to object oriented design only make sense if there’s a difference in understandability (e.g. using one class versus two).
It Uses an Extra Object
Some people object on the basis that the instance form creates an additional object. That’s true, but in practice it doesn’t matter. Rails requests and background jobs create thousands upon thousands of Ruby objects. Optimizing object creation to lessen the stress on the Ruby garbage collector is a legitimate technique, but do it where it counts. You can only find out where it counts by measuring. It’s inconceivable that the single additional object that the instance variant creates will have a measurable impact on the performance of your system. (If you have a counter example with data, I’d love to hear about it.)
It’s Cumbersome to Call
Finally, some object that it is just easier to type:
Job.perform(args)
# vs.
Job.new(args).perform
That is true. In that case I simply define a convenience class method that builds the object and delegates down. In fact, that is one of the few cases I think class methods are okay. You can have your cake and eat it too, in this regard.
Wrapping Up
Begin with an object instance, even if it doesn’t have state or multiple methods right away. If you come back to change it later, you (and your team) will be more likely to refactor. If it never changes, the difference between the class method approach and the instance is negligible, and you certainly won’t be any worse off.
There are very few cases where I am comfortable with class methods in my code. They are either convenience methods, wrapping the act of initializing an instance and invoking a method on it, or extremely simple factory methods (no more than two lines), to allow collaborators to more easily construct objects.
What do you think? Which form do you prefer and why? Are there any pros/cons that I missed? Post a comment and let me know what you think!
P.S. If this sort of thing interests you, and you want to read more articles like it, you might like the Code Climate newsletter (see below). It’s just one email a month, and includes content about refactoring and object design with a Ruby focus.
Further Reading
Thanks to Doug Cole, Don Morrison and Josh Susser for reviewing this post.