In two separate code reviews this week I dug up a trusty old article that I use when trying to refactor conditionals in Ruby. I've referred back to Ian Whitney's article Refactoring away a conditional so many times over the years.
Today, I thought I'd try explain this pattern myself based on some recent work.
The Problem: Sending Messages
Before I start I should note that I didn't do the work I've outlined below. Other talented engineers on my team implemented it! This post is inspired by our discussion in person and through code review. The code samples below are very much pseudo-code that I hammered out while writing this post and don't reflect the real code in our application.
In our app at work, we have a messaging interface where a director can send messages to a predefined mailing list. Those mailing lists can be:
- All centres that they manage.
- One of the centres they manage.
- All of the customers for one centre.
The director uses one interface to compose their message and chooses one of the three options above. When our application receives the request, it routes the data to the correct mailer to send the message to the recipients.
Like any code base that's been around for a while, the three different branches here all had different interfaces. One called a Sidekiq worker, and two of them called a MessageSender service with different params.
We were working on adding a new feature to this messaging system that scheduled messages to be sent at a future date. This meant that we needed to take some of the older code that was in a controller and put it in a background job.
There was LOTS of opportunity for refactoring here, but we really needed to keep the scope of this work under control.
So, how do we decide which of the existing message sending methods to use?
Rough Draft: A Message Handler
The first step was to gather all the parts in one place. Have you ever cleaned out old desk drawers? Well, this is a similar process. We found all the related parts and group them together so we could best decide what to do with them.
Here's an example of what that looked like. Note that I've simplified this to keep it short.
class ScheduledMessageHandler
attr_accessor :scheduled_message
def initialize(id)
@scheduled_message = ScheduledMessage.find(id)
end
def call
if to_customers?
CustomerMessageWorker.perform_async(customer_message_params)
else
CentreMessageSenderService.call(centre_message_params)
end
end
private
def to_customers?
scheduled_message.send_to == 'Customers'
end
def customer_message_params
# Extract necessary params
end
def centre_message_params
params = scheduled_message.slice(:message, :subject)
to_all_centres? ? params.merge(centre_ids: centre_ids) : params.merge(centre_id: centre_id)
end
def to_all_centres?
scheduled_message.send_to == 'All Centres'
end
def centre_ids
# Determine centre ids
end
def centre_id
# Determine centre id
end
end
Great! We have all the parts we need assembled in one place here. Lets take a look at what this is doing.
What are we actually doing?
At a glance, it looks like this class is deciding between two different classes, right?
But! There's conditional hidden later on. Take a look at this flow chart. First we decide which class to use, and if we end up in the branch that sends to centres, we decide what the params should look like.
A real good hint that this ScheduledMessageHander knows too much is that there are two separate methods that end in params, and only one of them will get used any time we call this class.
A Refactor: Three Classes with the Same Interface
Here's where we can use a Factory to refactor away this conditional. The trick, though, is that the factory needs to return an object with the same interface no matter which branch of the decision tree we end up at.
Since we're trying to isolate these changes because we don't have time to pull apart all of the CustomerMessageWorker and CentreMessageSenderService classes, let's make sure they have the same interface.
class CustomerMessageSender
def initialize(message_params = {})
@message_params = message_params
end
def call
CustomerMessageWorker.perform_async(message_params)
end
private
def message_params
# Extract necessary params
end
end
Ok, so now we have two MessageSender classes that have the same call interface. We can pass all of the message_params into the CustomerMessageSender and it knows which params it needs to call the worker.,
Next, lets look how we use the CentreMessageSenderService class. We don't want to crack that open for a refactor, but we do know that it does two things: 1. Sends messages to one centre, and 2. Sends messages to many centres.
Lets do something similar where we wrap the param extraction logic into two separate classes.
class CentreMessageSender
def initialize(message_params = {})
@message_params = message_params
end
def call
CentreMessageSenderService.call(message_params)
end
private
def message_params
message_params.slice(:message, :subject).merge(centre_id: centre_id)
end
def centre_id
# Determine centre_id
end
end
class CentresMessageSender
def initialize(message_params = {})
@message_params = message_params
end
def call
CentreMessageSenderService.call(message_params)
end
private
def message_params
message_params.slice(:message, :subject).merge(centre_ids: centre_ids)
end
def centre_ids
# Determine centre_ids
end
end
Message Sender Factory: Deciding which Class we Need
Now we have three classes that have the same interface. We can pass all of the message parameters into any one of the classes and they will extract the parameters they need and send the message!
But, how do we pick the right class? Remember those conditionals we were talking about?
Let's start by adding a match? method to each class that tells us if it matches the conditions for using that class.
class CustomerMessageSender
def self.match?(send_to)
send_to == 'Customers'
end
end
class CentreMessageSender
def self.match?(send_to)
send_to == 'Centre'
end
end
class CentresMessageSender
def self.match?(send_to)
send_to == 'All Centres'
end
end
And finally, lets make a factory class that can find the right message sender for us:
class MessageSenderFactory
MESSAGE_SENDERS = [
CustomerMessageSender,
CentreMessageSender,
CentresMessageSender,
]
def self.build(message_params)
send_to = message_params[:send_to]
klass = MESSAGE_SENDERS.find { |sender| sender.match?(send_to) }
klass.new(message_params)
end
end
Our factory class is now using the match? class method on each of the message sender classes to decide which class to use. It then initializes a new class and returns it to us.
Here's what using it looks like now:
message_sender = MessageSenderFactory.build(message_params)
message_sender.call
Notes and Thoughts
The original article I referenced above is called "Refactoring away a conditional", which seems correct because we're no longer using if else statements. But! We still have conditions, which live in the match? method. The difference here is that we've flattened the decision tree. Now we make one decision with three possible outcomes rather than two decisions with three possible outcomes.
Naming is Hard!
The names in my examples don't actually match what is in our application. I find the similarity between CentreMessageSender and CentresMessageSender really hard to distinguish. Perhaps I could spend some time picking better names here.
Deciding when to Refactor is Hard!
It is! I would have loved to revamp some of the classes behind this work. But the scope of that work would have been pretty significant in comparison to the project we were working on.
Refactoring an old code base is a little like tidying your house one day at a time. Want to read about that? Here's an amazing thread from Sarah Mei about code hoarding:
I've been thinking about how the konmari method applies to codebases. I've been doing a lightweight version of it with my physical stuff.
— Sarah Mei (@sarahmei) October 4, 2016
Top comments (0)