tl; dr
controllerでは特に、インスタンス変数の濫用はやめよう！
すべてのはじまり
ほとんど触ったことないプロジェクトに移籍させられ早1ヶ月、そのコードを知る熟練の先輩も去った荒廃の地で、私は今日も一人、Railsと激闘を繰り広げていた！
というわけで(？)、今回はcontrollerでやってはいけないアンチパターンを紹介していきたい。
クソコード1. インスタンス変数に入れればええやろ
これは初心者がやらかしがちな印象
こういうやつ
class SampleController < ApplicationController
def show
hoge = Hoge.new
@hoge_kuso1 = hoge.kuso
@hoge_kuso2 = hoge.sugoku_kuso
@hoge_kuso3 = Hoge.fetch_kuso
end
end
......
こ☆ろ☆す☆ぞ
まず第一に、Railsのコントローラーは一つのディレクトリに入るものをだいたい一つのクラスで固めるので、言い換えれば複数のアクションのロジックが一つのクラスにまとまっている。なので例えば次のようなコードは日常茶飯事である。
class SampleController < ApplicationController
def action1
end
def action2
end
end
この状態で2つのアクションがインスタンス変数をたくさん使ったとしよう。それが以下
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
ついでに Hoge クラスはこう
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
この状態の何が悪いのかというと、インスタンス変数をたくさん使ったことにより、コード全体でどの変数がどこに所属しているのかが分かりにくいことだ。例えば @hoge_kuso1 はクラス内スコープの変数であるので、もしかしたら他のところの定義とコンフリクトしてバグるかもしれない。それにクラス内検索しても複数の全然関係ない奴が引っかかる可能性もあるので、補足もしづらい。あんまりメリットはないのである。
解決策
単純な話、極力インスタンス変数を使わないことだ。具体的にはこう。
class SampleController < ApplicationController
def action1
@hoge = Hoge.new
end
def action2
@hoge = Hoge.new
end
...
end
require 'rest-client'
class Hoge
def kuso
:kuso
end
def sugoku_kuso
"sugoku kuso"
end
# 要素をモデルからアクセスできるようにして、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
どういうことかというと、インスタンス変数に入れるのはモデル程度にしておいて、詳細なデータはモデル内に内包してしまおう、ということだ。これのメリットは2つ
- コントローラーがスッキリする
- ロジックが複数の場所に依らない
正直メリットしかないので、やらない手はないと思う。ちなみにたとえば複数のモデルに対して一括で値を取得したいんだけど、その場合はどうするの？ということについては
class SampleController < ApplicationController
def action1
@hoges = 10.times.map { Hoge.new }
Hoge.set_kusos(@hoges)
end
end
require 'rest-client'
class Hoge
def kuso
:kuso
end
def sugoku_kuso
"sugoku kuso"
end
attr_accessor :fetched_kuso
# 専用のセッター
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
とするなどして、モデル内にデータを保持するといい。とにかくコントローラーで無闇にロジックを書かないようにすることは大事である。
クソコード2. set_〇〇って書いとけば安泰やろ。其の壱
ダメです
例えばこう書く人はほとんどだと思う
class SampleController < ApplicationController
before_action :set_hoge, only: :action1
def action1
end
private
def set_hoge
@hoge = Hoge.new
end
end
絶対10人に9人はこう書いてる。なぜならscaffoldでもこう書くのを推奨してるから。
じゃあこれの何がダメなのかというと、以下の2つ
- コードが追いにくい
- 引数を渡しにくい
まず1について
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
こう書かれた場合に、action1を確認した瞬間、あ、これは @hoge を使うんだな、ってなる人はいないだろう。
少なくとも、
action1がなにも定義されていない -> あ、before_actionが定義されているということはつまり -> やっぱり @hoge を使っていたか
となるのではないだろうか？
これはアハ体験としては適切な体験かもしれないが、コーディングに脳科学を持ち込まないでほしい。はっきり言ってストレスである。
また、 [クソコード１](#クソコード1. インスタンス変数に入れればええやろ)でも述べたように、コントローラー内で使用されるインスタンス変数は所在がわかりにくくなりがちである。よってインスタンス変数を無闇に使うのはよろしくない。
次に2について
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
こうしないと変数が渡せない。逆にこれだったら渡せるじゃん？という諸君。確かに渡せるが、これだとせっかく便利なskip_before_actionが利用できない。そこまでしてset_〇〇を使う必要ある？という気持ちでいっぱいである。
解決策
答えは単純である。
class SampleController < ApplicationController
def action1
@hoge = fetch_hoge
end
private
def fetch_hoge
Hoge.new
end
end
これであれば、インスタンス変数がロジック内に組み込まれているので、見た瞬間に @hoge を使うことがわかる。また引数を渡すのも容易である。とってもスマート、故にハッピー。
また、これだとredirect_toを書くのに手間がかかるじゃん！そっちはどうすんのさ！？っていうみなさんのために、以下のコードを授けよう
class SampleController < ApplicationController
class DataNotFound < RuntimeError; end
rescue_from DataNotFound do
redirect_to :root_path, flash: { error: 'データが見つかりませんでした' }
end
def action1
@hoge = fetch_hoge
end
private
def fetch_hoge
hoge = Hoge.new
raise DataNotFound unless hoge
hoge
end
end
これであれば元々の機能を維持できるだろう。
クソコード3. set_〇〇って書いとけば安泰やろ。其の弐
ダメだって言ってんだろ！！！
場合にもよるけど、例えばヘッダーで使う値をこうやって作る人いるよね
class SampleController < ApplicationController
before_action :set_header_de_tukau_yatu
def set_header_de_tukau_yatu
@header_info = 'void'
end
end
これの問題点は「@header_info を利用する」ためには、「:set_header_de_tukau_yatuを呼び出す」必要があることを知っていなきゃいけないことだ。この場合、viewがほしいのは @header_info だけ、なのになぜその設定方法までこっちが把握してなきゃいかんの？っていう話
解決策
こういうときにどうすればいいのかというと helper_methodを使う。例えば以下
class SampleController < ApplicationController
helper_method :header_info
def header_info
@header_info ||= 'void'
end
end
こうすることで、viewは header_info を呼び出すと、情報が得られる。ということを知っているだけでいい。わざわざ内部のロジックまで知る必要もないし、インスタンス変数もロジックに近い場所にある。とっても素晴らしい。
おわりに
この記事ではアンチパターンを3つしか紹介していないが、多分世の中にはもっとたくさんのアンチパターンがあると思う。しかし多くの場合、それは「可読性が下がる」「使い勝手が悪い」のいずれかないし両方を満たしているのではなかろうか？そしてその二つを生み出しがちな諸悪の根源、それはインスタンス変数の濫用である(暴論。今回本当に伝えたかったことはただ一つ。みなさんご唱和ください。
インスタンス変数の濫用はやめよう！！！！！
読了ありがとうございました。