[Rails]コントローラのbefore_actionでインスタンス変数を定義するのはやめよう

前提

この記事で書いているインスタンス変数は全てview側で参照しているものとします。

まずは結論から

↓のように、view側で使うインスタンス変数をbefore_actionで初期化するのはやめましょう。

class PostsController < ApplicationController
  before_action :set_post, only: [:show, :edit]

  def show; end

  def edit; end

  private

  def set_post
    @post = Post.find params[:id]
  end
end

なぜ何故よくないのか

view側でインスタンス変数が呼び出されてるのを見たとき、普通は真っ先にコントローラーのアクションを見に行きます。
その時にアクションの中でインスタンス変数が初期化されてない場合、どこで初期化されたか探す必要が出るためコードの可読性が下がります。
↑のコードはまだ一目見てわかるようになっていますが、例えば、

class PostsController < ApplicationController
  before_action :set_post, only: [:show, :edit]

  def show; end

  def edit; end

  # ここに行数が長いコード

  private

  # ここに行数が長いコード

  def set_post
    @post = Post.find params[:id]
  end
end

このように行数の長いコードが挟まってると、画面には収まりきらずどこでインスタンス変数を初期化したか探す羽目になります。

上記のコードの場合、コントローラ名、インスタンス変数名共に post を使ってるので、Railsに慣れてる方なら「多分before_actionだろうな」とすぐに気が付きますが、以下のように、

class PostsController < ApplicationController
  before_action :set_post, only: [:show, :edit]
  before_action :set_foo, only: [:show, :edit]
  before_action :set_bar, only: [:show, :edit]

  def show; end

  def edit; end

  # ここに行数が長いコード

  private

  # ここに行数が長いコード

  def set_post
    @post = Post.find params[:id]
  end

  def set_foo
    @foo = # ここに @foo 初期化処理
  end

  def set_bar
    @bar = # ここに @bar 初期化処理
  end
end

こうなってくると、Railsに慣れてる方でも探すのに苦労するようになってきます。

これならまだ set_ のプレフィックスがあるので何とか付いていけますが、例えば check_role みたいなbefore_actionで @role インスタンス変数を初期化していると、本格的にわけわからなくなってきます。（実際にこのような書き方でインスタンス変数探すのに苦労した案件が一部あります。）

解決案1

やはり、アクションで必要なインスタンス変数を初期化するのが王道でしょうか。

class PostsController < ApplicationController
  def show
    @post = get_post(params[:id])
    @foo = get_foo
    @bar = get_bar
  end

  def edit
    @post = get_post(params[:id])
    @foo = get_foo
    @bar = get_bar
  end

  # ここに行数が長いコード

  private

  # ここに行数が長いコード

  def get_post(id)
    Post.find id
  end

  def get_foo
    # ここに foo 取得処理
  end

  def get_bar
    # ここに bar 取得処理
  end
end

解決案2

または、以下のようにhelper_methodにしてしまってもよいと思います。

class PostsController < ApplicationController
  helper_method :post
  helper_method :foo
  helper_method :bar

  def show
  end

  def edit
  end

  # ここに行数が長いコード

  private

  # ここに行数が長いコード

  def post
    @post ||= Post.find params[:id]
  end

  def foo
    # ここに foo 取得処理
  end

  def bar
    # ここに bar 取得処理
  end
end

そしてview側では インスタンス変数ではなくヘルパーメソッド を呼び出します。

views/posts/show.html.erb

<p><%= post.body %></p>

<p><%= foo %></p>

<p><%= bar %></p>

これだと「before_action使ってる時とあまり変わらないじゃないか」と思う方もいるかもしれませんが、
helper_method はviewから呼び出すことが前提のメソッドなので、インスタンス変数初期化以外の用途でも使う before_action よりは「あ、これview側で使うんだな」ということがぱっと見で伝わります。

また、helper_method を使った場合、view側がインスタンス変数に依存しなくなるので、例えば @pest のようなtypoをやらかしてもその場でエラーを吐くのですぐに気が付けます。

まとめ

システムの規模が大きくなるとbefore_actionで初期化したインスタンス変数を探すのがしんどくなるのでやめよう。