Pimp My Ruby

Posted on Nov 29, 2023

Pimp My Code : Come and Clean Code with V #1

#ruby #rails #programming #tutorial

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 and Success are very expressive regarding the action's status. Failure can contain the error message. In the controller, we use Dry::Matcher, which allows dynamic reactions based on the type of result returned by CalendlyEvent.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 the case 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)

Ted Martinez • Nov 30 '23

You keep using .call like a class method, but it's defined as an instance method, is there a reason for that?

Pimp My Ruby • Nov 30 '23

Yes, it's a little oversight on my part. Actually all my Services inerhit from a base class called ApplicationService

require 'dry/monads'

class ApplicationService
  include Dry::Monads[:result, :do, :maybe, :try]

  def self.call(...)
    new(...).call
  end
end

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

DEV Community

Pimp My Code : Come and Clean Code with V #1

Starting Point

Analysis of Anti-patterns

1. Params Validation

2. How we manage the response of the Service

3. File Naming

4. Class Naming

5. Use `dig` if you're afraid

6. Say Goodbye to the If Blocks Hell

7. Duplicate Method Call

8. Lack of Clarity on Return

What do the Linters Say?

Rubocop

Reek

Rails Best Practices

My Updated Proposal

Tips of the Day

Conclusion

Top comments (2)

Read next

How to visualise MediaPipe’s Hand Tracking in 2D and 3D with Rerun

21 bugs in 21st version of Apache NetBeans

Why Magento is the #1 Choice of eCommerce Startups?

Selection sort in Python

Starting Point

Analysis of Anti-patterns

1. Params Validation

2. How we manage the response of the Service

3. File Naming

4. Class Naming

5. Use dig if you're afraid

6. Say Goodbye to the If Blocks Hell

7. Duplicate Method Call

8. Lack of Clarity on Return

What do the Linters Say?

Rubocop

Reek

Rails Best Practices

My Updated Proposal

Tips of the Day

Conclusion

Read next

How to visualise MediaPipe’s Hand Tracking in 2D and 3D with Rerun

21 bugs in 21st version of Apache NetBeans

Why Magento is the #1 Choice of eCommerce Startups?

Selection sort in Python

5. Use `dig` if you're afraid