Three Controller anti-patterns that we want to convey to Rails beginners to intermediates

tl; dr Stop abusing instance variables, especially with controllers!

The beginning of everything

One month after I was transferred to a project I had hardly touched, I was still fighting with Rails alone in the devastated land where the skilled seniors who knew the code had left! So (?), This time I would like to introduce anti-patterns that should not be done with the controller.

Fucking code 1. If you put it in an instance variable, yeah

This is an impression that beginners tend to do Such a guy

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 ☆ Ro ☆ Su ☆ zo **

First of all, Rails controllers consolidate what goes into one directory into one class, in other words, the logic of multiple actions in one class. So, for example, the following code is a daily occurrence.

sample_controller.rb


class SampleController < ApplicationController
  def action1
  end

  def action2
  end
end

Suppose the two actions use a lot of instance variables in this state. It is below

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

By the way, the Hoge class is like this

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 variable belongs to where in the whole code because of the heavy use of instance variables. For example, @ hoge_kuso1 is an in-class scope variable, so it may conflict with definitions elsewhere and cause a bug. Also, even if you search in the class, there is a possibility that multiple unrelated people will be caught, so it is difficult to supplement. There is not much merit.

solution

The simple story is to avoid using instance variables as much 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 elements accessible from the model to minimize 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 this means is that you should put only the model in the instance variables, and include the detailed data in the model. There are two benefits to this

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

To be honest, there is only merit, so I don't think there is anything I can do. By the way, for example, I want to get the values for multiple models at once. What should I do in that case? About 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 keep the data in the model. Anyway, it is important not to write logic indiscriminately with the controller.

Fucking code 2. If you write set_〇〇, it's safe. That one

Is useless For example, I think most people write like this

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 9 out of 10 write: Because scaffold also recommends writing like this. Then, what's wrong with this is the following two

  1. The code is hard to follow
  2. 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

When this is written, the moment you check action1, no one will say that this uses @ hoge. at least, action1 is not defined at all-> Ah, before_action is defined means-> After all, was @ hoge used? Isn't it? This may be a good experience for an aha experience, but don't bring brain science into your coding. To be clear, it's stress.

Also, as mentioned in [Fucking Code 1](#Fucking Code 1. If you put it in an instance variable), the location of the instance variable used in the controller tends to be difficult to understand. Therefore, it is not good to use instance variables indiscriminately.

Next 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 won't be able to pass variables. On the contrary, if this is the case, can you give it? You guys. You can pass it, but you can't use the convenient skip_before_action. Is it necessary to use set_〇〇 up to that point? 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

In this case, you can see that the instance variable is embedded in the logic, so you can use @ hoge the moment you see it. It's also easy to pass arguments. Very smart and therefore happy.

Also, with this, it takes time to write redirect_to! What are you doing over there! ?? Let's give the following code for everyone

sample_controller.rb


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

  def action1
    @hoge = fetch_hoge
  end

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

This will keep the original functionality.

Fucking code 3. If you write set_〇〇, it's safe. Part 2

You said no! !! !! It depends on the case, but for example, there are people who make the value used in the header 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, I only want the view @ header_info, so why do I have to figure out how to set it? The story

solution

What to do in such a case is to use helper_method. For example:

sample_controller.rb


class SampleController < ApplicationController
  helper_method :header_info

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

That way, view can get the information by calling header_info. All you have to do is know that. You don't have to bother to know the internal logic, and the instance variables are close to the logic. Very nice.

in conclusion

This article only introduces three anti-patterns, but I think there are probably more anti-patterns out there. But in many cases, isn't it either "less readable" or "unusable" or both? And the root of all the evils that tend to produce the two is the abuse of instance variables (arguing. There is only one thing I really wanted to convey this time. Please sing.

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

Thank you for reading.

Recommended Posts

Three Controller anti-patterns that we want to convey to Rails beginners to intermediates
When you want Rails to disable a session for a specific controller only
Three selections by good beginners who want to recommend to fledgling Swift engineers