[Ruby] 3 Controller Anti-Patterns that I want to convey from Rails beginners to intermediate users

5 minute read

tl; dr

Especially in controller, let’s stop the abuse of instance variables!

The beginning of everything

It was a month after I was transferred to a project I had hardly touched, and in the devastated land where even the seasoned seniors who knew the code had left, I was still fighting with Rails today! That’s why (?), so I would like to introduce anti-patterns that should not be done with controller this time.

Fucking code 1. Put it in an instance variable

This is an impression that beginners tend to do Something like this

sample_controller.rb


class SampleController <ApplicationController
  def show
    hoge = Hoge.new
    @hoge_kuso1 = hoge.kuso
    @hoge_kuso2 = hoge.sugoku_kuso
    @hoge_kuso3 = Hoge.fetch_kuso
  end
end

This☆☆☆

First of all, a Rails controller packs things in one directory into roughly one class, so in other words, the logic for multiple actions is in one class. So, for example, the following code is commonplace.

sample_controller.rb


class SampleController <ApplicationController
  def action1
  end

  def action2
  end
end

Suppose two actions use a lot of instance variables in this state. That is

sample_controller.rb


class SampleController <ApplicationController
  def action1
    hoge = Hoge.new
    @hoge_kuso1 = hoge.kuso
    @hoge_kuso2 = Hoge.fetch_kuso
  end

  def action2
    hoge = Hoge.new
    @hoge_kuso1 = hoge.sugoku_kuso
    @hoge_kuso2 = Hoge.fetch_kuso
  end

  ...
end

Here’s the Hoge class

hoge.rb


require'rest-client'

class Hoge
  def kuso
    :kuso
  end

  def sugoku_kuso
    "sugoku kuso"
  end

  def self.fetch_kuso
    RestClient.get('https://kuso.com/kuso3').body rescue'honma kuso'
  end
end

What’s wrong with this situation is that it’s hard to tell which variables belong to where in the whole code, because we used a lot of instance variables. For example, @hoge_kuso1 is an in-class variable, so it might conflict with other definitions and cause a bug. Also, even if you search in the class, multiple unrelated people may get caught, so it is difficult to supplement. There is not much merit.

solution

The simple story is to use instance variables as little as possible. Specifically:

sample_controller.rb


class SampleController <ApplicationController
  def action1
    @hoge = Hoge.new
  end

  def action2
    @hoge = Hoge.new
  end

  ...
end

hoge.rb


require'rest-client'

class Hoge
  def kuso
    :kuso
  end

  def sugoku_kuso
    "sugoku kuso"
  end

  Make the # element accessible to the model to eliminate the possibility of using instance variables on the controller.
  def fetched_kuso
    @fetched_kuso ||= self.class.fetch_kuso
  end

  def self.fetch_kuso
    RestClient.get('https://kuso.com/kuso3').body rescue'honma kuso'
  end
end

What I mean is that you put only instance variables in the model, and include detailed data in the model. There are two advantages to this

  1. The controller is refreshing
  2. Logic does not depend on multiple places

To be honest, there are only merit, so I don’t think there is a way to do it. By the way, I want to get the values for multiple models at once. What should I do in that case? Regarding that

sample_controller.rb


class SampleController <ApplicationController
  def action1
    @hoges = 10.times.map {Hoge.new}
    Hoge.set_kusos (@hoges)
  end
end

hoge.rb


require'rest-client'

class Hoge
  def kuso
    :kuso
  end

  def sugoku_kuso
    "sugoku kuso"
  end

  attr_accessor :fetched_kuso
  # Dedicated setter
  def self.set_kusos(hoges)
    hoges.each do |hoge|
      hoge.fetched_kuso = self.fetch_kuso
    end
  end

  def self.fetch_kuso
    RestClient.get('https://kuso.com/kuso3').body rescue'honma kuso'
  end
end

It is good to retain data in the model, for example. Anyway, it’s important not to write logic on the controller.

Fucking code 2. If you write set_XX, it’s safe. That one

Is useless For example, I think most people write

sample_controller.rb


class SampleController <ApplicationController
  before_action :set_hoge, only: :action1

  def action1
  end

  private
  def set_hoge
    @hoge = Hoge.new
  end
end

Absolutely nine in ten wrote: Because even scaffold recommends to write like this. Then what’s wrong with this is the following two

  1. Code is hard to follow
  2. It is difficult to pass arguments

First about 1

sample_controller.rb


class SampleController <ApplicationController
  before_action :set_hoge, only: :action1
  before_action :set_hoges, only: :action2

  def action1
  end
  def action2
  end

  private
  def set_hoge
    @hoge = Hoge.new
  end

  def set_hoges
    @hoges = [Hoge.new, Hoge.new]
  end
end

If it were written like this, no one would say that the moment you confirm action1, it uses @hoge. at least, action1 is not defined at all -> Ah, before_action is defined means -> After all, I used @hoge Will it be? This may be a good Aha experience, but don’t bring brain science into coding. To be clear, it’s stressful.

Also, as mentioned in Sucking code 1, the location of the instance variable used in the controller tends to be hard to find. Therefore, it is not good to use instance variables indiscriminately.

Then about 2

sample_controller.rb


class SampleController <ApplicationController
  before_action(only: :action1) do
    set_hoge('fuga')
  end

  def action1
  end

  private
  def set_hoge(fuga)
    @hoge = Hoge.new(fuga)
  end
end

If you don’t do this, you can’t pass variables. On the contrary, if this is the case, can you pass it? You guys. Certainly it can be passed, but with this it is not possible to use the convenient skip_before_action. Is it necessary to use set_○○ after that? I am full of feelings.

solution

The answer is simple.

sample_controller.rb


class SampleController <ApplicationController
  def action1
    @hoge = fetch_hoge
  end

  private
  def fetch_hoge
    Hoge.new
  end
end

If this is the case, you’ll see that you use @hoge the instant you see it, because the instance variables are embedded in the logic. It’s also easy to pass arguments. Very smart, so happy.

Also, it takes a lot of time to write redirect_to! How is that? ? Give everyone the following code for you

sample_controller.rb


class SampleController <ApplicationController
  class DataNotFound <RuntimeError; end
  rescue_from DataNotFound do
    redirect_to :root_path, flash: {error:'Data not found'}
  end

  def action1
    @hoge = fetch_hoge
  end

  private
  def fetch_hoge
    hoge = Hoge.new
    raise DataNotFound unless hoge
    hogeend
end

This will keep the original functionality.

Fucking code 3. If you write set_XX, it’s safe. That second

You’re saying no! !! !! Depending on the case, for example, there are people who create values used in headers like this

sample_controller.rb


class SampleController <ApplicationController
  before_action :set_header_de_tukau_yatu

  def set_header_de_tukau_yatu
    @header_info ='void'
  end
end

The problem with this is that you have to know that you need to “call :set_header_de_tukau_yatu” in order to “use @header_info”. In this case, the view only needs @header_info, so why should I know how to set it? The story

solution

The helper_method is used in this case. For example

sample_controller.rb


class SampleController <ApplicationController
  helper_method :header_info

  def header_info
    @header_info ||='void'
  end
end

By doing this, the view can get information by calling header_info. All you have to do is know that. You don’t even have to know the internal logic, and the instance variables are close to the logic. Very nice.

in conclusion

This article only mentions three anti-patterns, but I think there are many more anti-patterns out there. But in many cases, it’s either “less readable” or “inconvenient” or both? And the root of all the evils that tend to give rise to the two is the abuse of instance variables (abusive argument. The only thing I really wanted to tell you this time.

**Stop abuse of instance variables! !! !! !! !! **

Thank you for reading.