I started coding in Ruby four years ago. In these four years, I've built many applications and made a lot of mistakes.
What if today we take a little trip back in time to learn from these mistakes? We'll take a piece of code I wrote at the beginning of my Ruby career, analyze it to discover all the hidden anti-patterns, and then propose an update with the best practices I use every day.
Get ready because today, we're diving into some really dirty code!
Starting Point
Today, we'll analyze code produced during my very first freelance mission in 2021. The class we're looking at today has a mission: to handle the Calendly webhook. The feature is simple: when a Calendly event is created, we want to update the calendly_event_url
attribute of theAccount
related to the person that books an appointment. Nothing too complicated, right?
Let's see how my past self solved this problem:
Our API will expose an endpoint for Calendly to send us its information:
# app/controllers/api/v1/webhooks_controller.rb
module Api
module V1
class WebhooksController < ApiController
def calendly
was_handled = CalendlyEvent.call(request.body.string)
render json: { was_handled: was_handled }, status: :ok
end
end
end
end
We call a service CalendlyEvent
, let's take a look at its code:
# app/services/CalendlyEvent.rb
class CalendlyEvent
def call(payload)
@payload = JSON.parse(payload)
email = @payload['payload']['email']
if email.present?
@account = Account.find_by(email: email)
if @account.present?
update_account
end
end
end
private
def update_account
case @payload['event']
when 'invitee.created'
@account.update(is_calendly_meet_taken: true)
@account.update(calendly_event_url: @payload['payload']['uri'])
when 'invitee.canceled'
@account.update(is_calendly_meet_taken: false)
@account.update(calendly_event_url: nil)
end
end
end
And ... no tests! I never wrote tests back then because I thought it was unnecessary. Times have changed!
Before we go further, take some time to analyze this code. What bothers you? What do you want to improve?
There's no wrong answer; the code is so bad that whatever you do will improve it!
Let's go through all the comments I have for my past self.
Analysis of Anti-patterns
Let's start by analyzing our WebhooksController
.
1. Params Validation
The way our controller sends information to the service is not clean at all. By using request.body.string
, I don't validate how Calendly's payload works. Also, I don't need to pass all the information to my service. It only needs 3 pieces of information. Always permit the params before processing them:
# app/controllers/api/v1/webhooks_controller.rb
[...]
def calendly
was_handled = CalendlyEvent.call(calendly_event_params)
[...]
def calendly_event_params
params.permit(:event, payload: %i[email uri])
end
This way, in our class, we can extract the parameters by using destructuring:
# app/services/CalendlyEvent.rb
[...]
def call(event:, payload:)
[...]
In the payload
parameter, we'll have a hash with two keys, :email
and :uri
.
If we want to go further, we can use a Data Transfer Object (DTO). This object will contain all the attributes we need in the service's processing.
For example:
# app/dto/calendly/event_received_dto.rb
module Calendly
class EventReceivedDTO
attr_accessor :event, :email, :uri
def initialize(event:, payload:)
@event = event
@email = payload[:email]
@uri = payload[:uri]
end
def account_update_body
case @event
when 'invitee.created'
{ is_calendly_meet_taken: true, calendly_event_url: @uri }
when 'invitee.canceled'
{ is_calendly_meet_taken: false, calendly_event_url: nil }
else
{}
end
end
end
end
# app/controllers/api/v1/webhooks_controller.rb
[...]
def calendly
calendly_event = Calendly::EventReceived.new(calendly_event_params)
was_handled = CalendlyEvent.call(calendly_event)
[...]
This approach is powerful because it allows us to abstract a lot of things and isolate the processing of the data sent by Calendly to expose a simple and intuitive interface.
2. How we manage the response of the Service
This is really a detail, but I have absolutely no idea how our response is made to Calendly. For your information, Calendly completely ignores our response. We could send { "foo": "bar" }
, and everything would still work. The only thing Calendly looks at is the HTTP code of our response. As long as it's in the 2XX range, everything is fine! But it's not normal that our API doesn't return details about the error.
Let's assume that we're sending this response to a front-end interface. Simply telling them, "No, it doesn't work," is not sufficient to display a correct error to the user.
So, we need to rethink how our service communicates errors to have more information. We'll see later how to do this 👀.
Now, let's look at the CalendlyEvent
class.
3. File Naming
The service is in a file called CalendlyEvent.rb
, but that's not the convention. All files should be in snake_case, not CamelCase. The filename should be calendly_event.rb
.
4. Class Naming
CalendlyEvent
doesn't tell us anything about what's happening inside. There's no action verb. If I want to stay generic, I would rename it to CalendlyEventProcessor
. And if I wanted to be very specific, I would name it LinkCalendlyEventToAccount
. Also, by convention, I always add the suffix Service
to the name of my services.
5. Use dig
if you're afraid
In the code:
@payload['payload']['email']
If we don't have a value for the payload
key, then we have:
nil['email']
Which, as you well know, triggers a NoMethodError: undefined method '[]' for nil:NilClass
. The solution to ensure no error is to use the .dig
method, which returns nil if we can't reach the desired value.
6. Say Goodbye to the If Blocks Hell
The biggest problem with this class is its nested if
statements, making the code challenging to read.
There are several ways to address this issue:
1- Inline Returns: I like this solution a lot, and I used it for a long time:
def call(payload)
@payload = JSON.parse(payload)
email = @payload['payload']['email']
return false if email.nil?
@account = Account.find_by(email: email)
return false if @account.nil?
update_account
end
Clearer, right?
2- Add Some Abstraction:
In reality, the actions we perform can be summarized as "Find the Account, if it exists, update it."
Our .call
method does too much.
"Find the email, if it doesn't exist, return false, find the Account, if it doesn't exist, return false, then update the Account."
We need to break down the responsibilities:
[...]
def call(payload)
@payload = JSON.parse(payload)
find_account
return false if @account.nil?
update_account
end
def find_account
@account = Account.find_by(email: @payload['payload']['email'])
end
[...]
Now, we have only one if
statement, which is fantastic! How can we get rid of it?
I have a fantastic tool for that – Monads!
1- Add Monads: If you're not familiar with Monads in Ruby, I highly recommend checking out my article on the subject.
Specifically, we want to abstract the return value of find_account
to handle only the success case:
def call(payload)
@payload = JSON.parse(payload)
yield find_account
update_account
Success()
end
def find_account
@account = Account.find_by(email: @payload['payload']['email'])
Maybe(@account).to_result(false)
end
And ta-da! No more if
statements in our code! It's much clearer, don't you think?
However, be aware that we need to update our controller to treat the result of our service as a Monad rather than a Boolean.
7. Duplicate Method Call
I have no idea why I did this, but I did:
when 'invitee.created'
@account.update(is_calendly_meet_taken: true)
@account.update(calendly_event_url: @payload['payload']['uri'])
when 'invitee.canceled'
@account.update(is_calendly_meet_taken: false)
@account.update(calendly_event_url: nil)
end
Performing an update with two method calls is something!
This is a terrible thing for several reasons:
- It makes 2 database transactions.
- We don't handle any error cases.
- It's 2 lines.
We can quickly fix this like so:
when 'invitee.created'
@account.update!(is_calendly_meet_taken: true, calendly_event_url: @payload['payload']['uri'])
when 'invitee.canceled'
@account.update!(is_calendly_meet_taken: false, calendly_event_url: nil)
end
But we still have a duplicated method call; we're still calling @account.update!
twice.
The problem here is that the case
block is in the wrong place. We shouldn't vary @account.update!
but its content. The case
block should help us determine the attributes to send to @account.update!
.
Here's an example fix:
def update_account
@account.update!(account_update_attributes)
end
def account_update_attributes
case @payload['event']
when 'invitee.created'
{ is_calendly_meet_taken: true, calendly_event_url: @payload['payload']['uri'] }
when 'invitee.canceled'
{ is_calendly_meet_taken: false, calendly_event_url: nil }
else
{}
end
end
And we no longer have a duplicated method call!
Another way to do this is to place our object in a constant, for example:
EVENT_TYPE_ACCOUNT_ATTRIBUTES_MAPPING = {
"invitee.created" => { is_calendly_meet_taken: true, calendly_event_url: @payload['payload']['uri'] },
"invitee.canceled" => { is_calendly_meet_taken: false, calendly_event_url: nil }
}.freeze
[...]
def update_user
@user.update!(EVENT_TYPE_ACCOUNT_ATTRIBUTES_MAPPING[@payload['event']])
end
Unfortunately, this doesn't work because @payload['payload']['uri']
is a dynamic value. We can't assign it to a constant that is freeze
.
8. Lack of Clarity on Return
Our .call
method returns a simple boolean. What question does this boolean answer?
If our class is named CalendlyEvent
, it's challenging to know for sure.
If our class is named CalendlyEventProcessor
, we might say, "My event has been processed," but that doesn't really add much meaning.
If our class is named LinkCalendlyEventToAccount
, we might say, "My Calendly event has been linked to my Account," which is a bit more explicit.
Most importantly, in the case of failure, why did it fail?
In concrete terms, what our method returns should be much more explicit about what happened. In general, returning a boolean, an error code, or a string is often insufficient to provide context to the caller. We almost always want to encapsulate the return value to provide as much context as possible.
-
Returning a Hash: Just like what we could do for API communication.
# app/services/calendly_event.rb [...] def call(payload) @payload = JSON.parse(payload) email = @payload['payload']['email'] return { success: false, message: "Email key is missing" } if email.nil? @account = Account.find_by(email: email) return { success: false, message: "Account not found" } if @account.nil? update_account { success: true } end [...] # app/controllers/api/v1/webhooks_controller.rb [...] def calendly render json: CalendlyEvent.call(request.body.string), status: :ok end
Of course, in a real application with a genuine front end, we would want to internationalize our error messages.
-
Returning a Monad: I will talk about this often; I love Monads.
# app/services/calendly_event.rb [...] def call(payload) @payload = JSON.parse(payload) email = @payload['payload']['email'] return Failure("Email key is missing") if email.nil? @account = Account.find_by(email: email) return Failure("Account not found") if @account.nil? update_account Success() end [...] # app/controllers/api/v1/webhooks_controller.rb [...] def calendly Dry::Matcher::ResultMatcher.call(CalendlyEvent.call(request.body.string)) do |matcher| matcher.success { render json: { success: true }, status: :ok } matcher.failure { |message| render json: { status: false, message: message }, status: :ok } end end
Here,
Failure
andSuccess
are very expressive regarding the action's status.Failure
can contain the error message. In the controller, we useDry::Matcher
, which allows dynamic reactions based on the type of result returned byCalendlyEvent.call
.
Wow, I had a lot to criticize about my past self! Before presenting my well-constructed proposal, I'd like us to look at what the linters have to say together.
To learn more about the linters I use, read this article.
What do the Linters Say?
Let's run a set of linters on the two files we've seen together, and let's see the feedback.
Rubocop
app/controllers/api/v1/webhooks_controller.rb
: ✅
app/services/calendly_event.rb
: ❌ We have 9 warnings, 8 of which are correctable.
Here's the version of Rubocop once all correctable warnings are addressed:
# app/services/CalendlyEvent.rb
class CalendlyEvent
def call(payload)
@payload = JSON.parse(payload)
email = @payload['payload']['email']
return if email.blank?
@account = Account.find_by(email: email)
return if @account.blank?
update_account
end
private
def update_account
case @payload['event']
when 'invitee.created'
@account.update!(is_calendly_meet_taken: true)
@account.update!(calendly_event_url: @payload['payload']['uri'])
when 'invitee.canceled'
@account.update!(is_calendly_meet_taken: false)
@account.update!(calendly_event_url: nil)
end
end
end
Not bad! I'll use this version from now on to pass the following linters.
Reek
app/controllers/api/v1/webhooks_controller.rb
: ✅
app/services/calendly_event.rb
: ❌ We have 4 warnings.
- InstanceVariableAssumption: Reek doesn't like that in the
.update_account
method, I call@account
and@payload
before testing if they have been defined. - IrresponsibleModule: I have no comment for my class; I agree.
- TooManyStatements: According to Reek, we make too many variable modifications in the
.call
method; I concede.
Rails Best Practices
app/controllers/api/v1/webhooks_controller.rb
: ✅
app/services/calendly_event.rb
: ✅
Yeah!
The linters have helped find other problems that are important to consider during the refactor.
Now that we've seen all of this, let's move on to my cleaning proposal!
My Updated Proposal
Without further ado, here's my updated proposal!
# app/dto/calendly/event_received_dto.rb
module Calendly
class EventReceivedDTO
attr_reader :event, :email, :uri
def initialize(event:, payload:)
@event = event
@email = payload[:email]
@uri = payload[:uri]
end
def account_update_attributes
case @event
when 'invitee.created'
{ is_calendly_meet_taken: true, calendly_event_url: @uri }
when 'invitee.canceled'
{ is_calendly_meet_taken: false, calendly_event_url: nil }
else
{}
end
end
end
end
# app/services/calendly/link_receved_event_to_account_service.rb
module Calendly
# When an account books an appointment on Calendly, we need to sync it with our database
# calendly_event_dto can be found in app/dto/calendly/event_received.rb
class LinkReceivedEventToAccountService
def call(calendly_event_dto:)
find_account(calendly_event_dto.email).fmap do |account|
account.update!(calendly_event_dto.account_update_attributes)
end
end
private
def find_account(email)
Maybe(
Account.find_by(email: email)
).to_result(:account_not_found)
end
end
end
# app/controllers/api/v1/webhooks_controller.rb
module Api
module V1
class WebhooksController < ApiController
def calendly
Dry::Matcher::ResultMatcher.call(call_calendly_service) do |matcher|
matcher.success { render json: { success: true }, status: :ok }
matcher.failure { |message| render json: { status: false, message: message }, status: :ok }
end
end
private
def call_calendly_service
Calendly::LinkReceivedEventToAccountService.call(Calendly::EventReceivedDTO.new(calendly_event_attributes))
end
def calendly_event_attributes
params.permit(:event, payload: %i[email uri])
end
end
end
end
Our service has really undergone a significant cleanup, and it's very satisfying!
- We no longer have a single
if
in our code. Apart from thecase
block, we have a very clear execution path. - The service does much less thanks to the introduction of the DTO.
- The controller provides a better response to the API, all without ternaries or
if
statements!
Tips of the Day
Now that we've cleaned up my past code, let's go through the applicable tips we've seen today in bullet point form:
1. Use of Objects Instead of Primitives:
- Encourage the use of dedicated objects, such as DTOs, to transport data. This promotes better encapsulation and cleaner parameter handling.
2. Prioritize Clarity Over Conciseness:
- Opt for clear code rather than excessive conciseness. Variable and method names should be expressive, even if it means they are a bit longer.
3. Use Naming Conventions:
- Adhere to Rails naming conventions to facilitate code understanding for other developers. Consistency is crucial.
4. Standardization of Error Messages:
- For a better user experience, standardize error messages returned by services. This makes debugging easier and provides more useful information to end users.
5. Encapsulation and Limitation of Responsibilities:
- Services should encapsulate specific actions and not expose too many details. This makes code maintenance and scalability easier.
6. Documentation and Comments:
- In theory, good code is code that is not documented. But in practice, you're happy to have a comment that explains the business context when the code was produced. Especially when it's code related to an external tool (like Calendly in our case).
Conclusion
What a journey through time! At first, I was a bit hesitant to show you this piece of code, but I have to admit that I have made good progress since 2021.
Have you ever done an exercise like this? In any case, I recommend it!
Tell me if you like the concept, and most importantly, give me all your feedback in the comments. What else would you do? Do you agree with my additions?
Write to me just below ⤵️
PS: If you want me to analyze your code to turn it into an article, send me a private message :)
Top comments (2)
You keep using .call like a class method, but it's defined as an instance method, is there a reason for that?
Yes, it's a little oversight on my part. Actually all my Services inerhit from a base class called ApplicationService
I only use initializer for inject the dependencies (other classes that needs to be called inside my class). But all the parameters related to the behavior of my class are submitted in the
.call
method