tl; dr

controllerでは特に、インスタンス変数の濫用はやめよう！

すべてのはじまり

ほとんど触ったことないプロジェクトに移籍させられ早1ヶ月、そのコードを知る熟練の先輩も去った荒廃の地で、私は今日も一人、Railsと激闘を繰り広げていた！
というわけで(？)、今回はcontrollerでやってはいけないアンチパターンを紹介していきたい。

クソコード1. インスタンス変数に入れればええやろ

これは初心者がやらかしがちな印象
こういうやつ

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

......

こ☆ろ☆す☆ぞ

まず第一に、Railsのコントローラーは一つのディレクトリに入るものをだいたい一つのクラスで固めるので、言い換えれば複数のアクションのロジックが一つのクラスにまとまっている。なので例えば次のようなコードは日常茶飯事である。

sample_controller.rb

class SampleController < ApplicationController
  def action1
  end

  def action2
  end
end

この状態で2つのアクションがインスタンス変数をたくさん使ったとしよう。それが以下

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

ついでに Hoge クラスはこう

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

この状態の何が悪いのかというと、インスタンス変数をたくさん使ったことにより、コード全体でどの変数がどこに所属しているのかが分かりにくいことだ。例えば @hoge_kuso1 はクラス内スコープの変数であるので、もしかしたら他のところの定義とコンフリクトしてバグるかもしれない。それにクラス内検索しても複数の全然関係ない奴が引っかかる可能性もあるので、補足もしづらい。あんまりメリットはないのである。

解決策

単純な話、極力インスタンス変数を使わないことだ。具体的にはこう。

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

  # 要素をモデルからアクセスできるようにして、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つ

コントローラーがスッキリする
ロジックが複数の場所に依らない

正直メリットしかないので、やらない手はないと思う。ちなみにたとえば複数のモデルに対して一括で値を取得したいんだけど、その場合はどうするの？ということについては

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
  # 専用のセッター
  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_〇〇って書いとけば安泰やろ。其の壱

ダメです
例えばこう書く人はほとんどだと思う

sample_controller.rb

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について

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

こう書かれた場合に、action1を確認した瞬間、あ、これは @hoge を使うんだな、ってなる人はいないだろう。
少なくとも、
action1がなにも定義されていない -> あ、before_actionが定義されているということはつまり -> やっぱり @hoge を使っていたか
となるのではないだろうか？
これはアハ体験としては適切な体験かもしれないが、コーディングに脳科学を持ち込まないでほしい。はっきり言ってストレスである。

また、 [クソコード１](#クソコード1. インスタンス変数に入れればええやろ)でも述べたように、コントローラー内で使用されるインスタンス変数は所在がわかりにくくなりがちである。よってインスタンス変数を無闇に使うのはよろしくない。

次に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

こうしないと変数が渡せない。逆にこれだったら渡せるじゃん？という諸君。確かに渡せるが、これだとせっかく便利なskip_before_actionが利用できない。そこまでしてset_〇〇を使う必要ある？という気持ちでいっぱいである。

解決策

答えは単純である。

sample_controller.rb

class SampleController < ApplicationController
  def action1
    @hoge = fetch_hoge
  end

  private
  def fetch_hoge
    Hoge.new
  end
end

これであれば、インスタンス変数がロジック内に組み込まれているので、見た瞬間に @hoge を使うことがわかる。また引数を渡すのも容易である。とってもスマート、故にハッピー。

また、これだとredirect_toを書くのに手間がかかるじゃん！そっちはどうすんのさ！？っていうみなさんのために、以下のコードを授けよう

sample_controller.rb

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_〇〇って書いとけば安泰やろ。其の弐

ダメだって言ってんだろ！！！
場合にもよるけど、例えばヘッダーで使う値をこうやって作る人いるよね

sample_controller.rb

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を使う。例えば以下

sample_controller.rb

class SampleController < ApplicationController
  helper_method :header_info

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

こうすることで、viewは header_info を呼び出すと、情報が得られる。ということを知っているだけでいい。わざわざ内部のロジックまで知る必要もないし、インスタンス変数もロジックに近い場所にある。とっても素晴らしい。

おわりに

この記事ではアンチパターンを3つしか紹介していないが、多分世の中にはもっとたくさんのアンチパターンがあると思う。しかし多くの場合、それは「可読性が下がる」「使い勝手が悪い」のいずれかないし両方を満たしているのではなかろうか？そしてその二つを生み出しがちな諸悪の根源、それはインスタンス変数の濫用である(暴論。今回本当に伝えたかったことはただ一つ。みなさんご唱和ください。

インスタンス変数の濫用はやめよう！！！！！

読了ありがとうございました。