Pimp My Code : Come and Clean Code with V #1
Tags: ruby, rails, programming, tutorial
Published: 2023-11-29 17:10:04

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 the`Account` 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:

```ruby
# 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:

```ruby
# 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:

```ruby
# 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:

```ruby
# 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:

```ruby
# 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:

```ruby
@payload['payload']['email']
```

If we don't have a value for the `payload` key, then we have:

```ruby
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:

```ruby
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:

```ruby
[...]
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](https://dev.to/justthev/mastering-monadical-syntax-in-ruby-a-practical-guide-to-functional-elegance-1c0g).
    
Specifically, we want to abstract the return value of `find_account` to handle only the success case:

```ruby
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:

```ruby
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:

```ruby
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:

```ruby
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:

```ruby
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.

1. **Returning a Hash:** Just like what we could do for API communication.
    
    ```ruby
    # 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.
    
2. **Returning a Monad:** I will talk about this often; I love Monads.
    
    ```ruby
    # 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](https://dev.to/justthev/mastering-linters-a-code-quality-assurance-comprehensive-guide-using-ruby-on-rails-5aa2).

---

# 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.


![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/h5njw2draw54oxzabl17.png)

Here's the version of Rubocop once all correctable warnings are addressed:

```ruby
# 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.


![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/n1zfj0z2y0w03cg1fokg.png)

- 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!

```ruby
# 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 :)