tl; dr Stop abusing instance variables, especially with controllers!
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.
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.
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
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.
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
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.
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.
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
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.
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