はじめに
コードレビューでよく指摘しているシリーズです。
たとえばRailsで以下のようなコントローラがあったとします。
class SomethingController < ApplicationController
before_action :set_something, only: %i[edit update destroy]
before_action :set_something_for_show, only: %i[show]
def index
# ...
end
# 他にもいろいろなアクション
private
def set_something
@something = Something.extra_condition.find(params[:id])
end
def set_something_for_show
@something = Something.find(params[:id])
end
end
このコードのポイントは以下の通りです。
- edit, update, destroyの場合は
set_somethingをbefore_actionで呼び出す - showの場合は（特別な検索条件が付かない）
set_something_for_showをbefore_actionで呼び出す
本記事では上のようなコードの良し悪しについて議論したいと思います。
このコードの何が問題か？
このコードのどこが問題なのでしょうか？
・・・いえ、特に致命的な問題はありません。
人によっては「これで全然OK」と考える人もいるでしょう。
ですが、僕がコードレビューするときは「set_something_for_showをなくしましょう」とコメントします。
その理由を以下で説明します。
before_actionのメリットとデメリット
before_actionのメリットは複数のアクションで呼び出される共通の前処理を定義できることです。
処理を共通化できるのでコードがDRYになります。
しかし、デメリットもあります。
before_actionが少ないうちはいいですが、before_actionが3つ、4つと増えていくうちに、処理の流れを追いかけるのがだんだん大変になっていきます。
なぜなら、before_actionを使うとコードを読むときに視線を大きく上下させないといけないからです。
ソースコードはなるべく上から下に読める方が人間にとって読みやすいです。
ですが、before_actionを使うと上から下に読むことが難しくなります。
onlyが1個だけのbefore_actionはメリットがない
このように、before_actionは共通の処理をDRYにできるというメリットがある一方、コードの見通しが悪くなる、というデメリットがあります。
このメリットとデメリットを把握した上で最初に提示したコードをもう一度見直してみるとどうでしょう？
class SomethingController < ApplicationController
# edit, update, destroyで前処理を共通化
before_action :set_something, only: %i[edit update destroy]
# これは何を共通化・・・？？？
before_action :set_something_for_show, only: %i[show]
def index
# ...
end
# 他にもいろいろなアクション
private
def set_something
@something = Something.extra_condition.find(params[:id])
end
def set_something_for_show
@something = Something.find(params[:id])
end
end
set_somethingはedit, update, destroyで前処理を共通化できていますが、set_something_for_showはshowでしか呼ばれないので、共通化するメリットがありません。
一方で、視線を上下に動かさないといけないデメリットは残ったままです。
before_actionをなくせば問題は解決！
このように、onlyの中身が1個だけのbefore_actionはデメリットしかないので定義する意味がありません。
set_something_for_showをなくせば、コードは以下のようになります。
class SomethingController < ApplicationController
before_action :set_something, only: %i[edit update destroy]
# ...
def show
@something = Something.find(params[:id])
# ...
end
# ...
private
def set_something
@something = Something.extra_condition.find(params[:id])
end
end
showでしか呼ばれない処理をshowに移動させた結果、showに関する処理はdef showの中身だけを確認すればよくなりました！
コードの見通しがよくなっただけでなく、ファイル全体の行数も減っていいことづくめですね👌
アクションが1個しかないコントローラの場合も同様
同様の理屈で、アクションが1個しかないコントローラもbefore_actionは不要です。
たとえば、以下のコントローラはshowアクションしかありません。
class SomethingController < ApplicationController
before_action :set_something_for_show
# このコントローラのアクションはshowだけ
def show
# ...
end
private
def set_something_for_show
@something = Something.find(params[:id])
end
end
before_actionにonlyオプションは付いていませんが、showアクションしか定義されていないのであれば、結果としてonly: %i[show]と書いているのと変わりません。
であれば、以下のようにbefore_actionをなくしてしまった方がベターです。
class SomethingController < ApplicationController
# before_actionをなくしてスッキリ！
def show
@something = Something.find(params[:id])
end
end
Rails初心者の方はset_foo_barみたいなメソッドは必ずbefore_actionで呼び出さなきゃいけないと考えがちですが、メリットとデメリットを正しく把握して、そういう固定観念はなくしましょう！
FAQ
ここまでの説明を読んで、疑問を感じた人もいるかもしれないので、以下でその疑問にお答えしましょう。
Q1. 今は1個だけど将来onlyの中身が増える可能性があるのでは？
今は1個だけど将来はonlyの中身が増えるかもしれないのでbefore_actionのままにしておきたい、と考える人がいるかもしれません。
しかし、それは実際にその必要性が出てきたときに共通の処理をbefore_actionに切り出せばいいです。
ソフトウエアは文字通り「ソフト」なので、いつでも簡単に修正できます。
いわゆるYAGNIっていうやつですね。
Q2. before_actionを使った方が一貫性があっていいと思うんですが？
インスタンス変数に値をセットするset_foo_barみたいなメソッドは必ずbefore_actionを使うようにする、とした方が実装に一貫性があって良い、と考える人もいるかもしれません。
たしかに、プログラムの動きとしてはbefore_actionを使う場合も使わない場合も同じなので、どっちを選ぶかはある意味好みの問題とも言えます。
もしあなたの開発チームが僕の記事を読んでも「いや、それよりも一貫性の方が大事だよね！実装を揃えた方がわかりやすいよね！」とみんな合意してくれるなら、before_actionを使い続けるという判断をしても構いません。
ただ、before_actionで呼び出されるメソッドは、いずれも副作用を期待するメソッド（＝戻り値が使われないメソッド）です。
set_somethingメソッドであれば、インスタンス変数@somethingになんらかの値がセットされる副作用を期待することになります。
RubyやRailsに限らず、プログラミングの一般論として、副作用が発生するメソッドはコードの見通しが悪くなるのでなるべく避けた方がいいものとされます。
もちろん、Railsアプリケーション開発ではメソッドの副作用を完全に排除することは不可能です。
ですが、それでも変に一貫性を大事にするより「before_actionは使わずに済むならなるべく使わないようにする（つまり副作用を持つメソッドをなるべく避ける）」とした方がコードの品質を向上できるはずだと僕は思うんですが、いかがでしょうか？
例外パターン
さて、ここまで「onlyの中身が1個だけのbefore_actionを定義するな！」という話をしてきましたが、状況によってはonlyの中身が1個だけの場合も許容できるかもしれません。
以下にそんな例外パターンを2つ紹介します。
例外1：処理を中断する系のbefore_actionなら残してもいいかも
上の例で示したような「インスタンス変数に値をセットする」系のbefore_actionは、デメリットしかないのでなくした方がいいですが、「特定の条件で処理を中断する」系のbefore_actionは例外的に残してもいいかもしれません。
たとえば、以下はshowアクションを実行するために特定の権限が必要になる場合のコード例です。
class SomethingController < ApplicationController
before_action :set_something, only: %i[edit update destroy]
# showアクションのみ権限チェックを実行
before_action :require_some_permission, only: %i[show]
# ...
def show
# 権限があればshowを表示
end
# ...
private
def set_something
@something = Something.extra_condition.find(params[:id])
end
def require_some_permission
# 権限がなければ処理を中断してroot_urlへリダイレクト
if no_permission?
flash[:error] = "権限がありません"
redirect_to root_url
end
end
end
メソッドの名前が適切であれば、before_actionの定義を見ただけで「あ、ここで権限チェックをしてるんだな。権限がなければshowできないんだろうな」想像できるので、メソッドの定義を見に行かなくても済みます。（予想に反する実装になっていない限り）
class SomethingController
before_action :set_something, only: %i[edit update destroy]
# あなた「おっ、showのときは何か特別な権限が必要みたいだな？」
before_action :require_some_permission, only: %i[show]
もちろん、before_actionにせず、showメソッドの中に同じ処理を書くのもありです。
ただし、before_actionとは違ってこの場合は必ずreturnする必要があります。
class SomethingController < ApplicationController
before_action :set_something, only: %i[edit update destroy]
# ...
def show
if no_permission?
flash[:error] = "権限がありません"
redirect_to root_url
# AbstractController::DoubleRenderErrorを避けるためにreturnする
return
end
# 権限があればshowを表示
end
# ...
private
def set_something
@something = Something.extra_condition.find(params[:id])
end
end
もしくはif/elseで分岐する必要があります。
が、この場合はインデントが深くなるデメリットがあります。
def show
if no_permission?
flash[:error] = "権限がありません"
redirect_to root_url
else
# 権限があればshowを表示（else節なのでインデントが1つ深くなる）
end
end
好みの問題なのでどれを選んでも間違いではありませんが、こういうケースはbefore_actionを残したままでもいいかもしれませんね。
class SomethingController < ApplicationController
before_action :set_something, only: %i[edit update destroy]
# onlyが1個でも可読性は変わらず、コードがシンプルになるのでこれはありかも？
before_action :require_some_permission, only: %i[show]
# ...
def show
# 権限があればshowを表示
end
# ...
private
def set_something
@something = Something.extra_condition.find(params[:id])
end
def require_some_permission
# 権限がなければ処理を中断
if no_permission?
flash[:error] = "権限がありません"
redirect_to root_url
end
end
end
例外2：親クラスで定義されているbefore_actionならonlyが1個でもOK
onlyの中身が1個だけのbefore_actionでも、それが自分のコントローラ内ではなく、親クラスで定義されているメソッドなら、そのままbefore_actionを残してもOKです。
class SomethingController < ApplicationController
before_action :set_something, only: %i[edit update destroy]
# set_something_for_show は ApplicationController で定義されているメソッド
before_action :set_something_for_show, only: %i[show]
def index
# ...
end
# ...
private
def set_something
@something = Something.extra_condition.find(params[:id])
end
# set_something_for_show の定義はここにはない
end
親クラスでbefore_actionで使うメソッドが定義されているということは、そのメソッドは他のコントローラでも使われるということです。
つまり、処理の共通化というメリットがあるので、この場合はbefore_actionを残してもOK、ということになります。
とはいえ、こういうケースはほとんど見かけない
とはいえ、例外1も2も、僕の経験上あまり登場頻度は高くありません。
ですので、onlyが1個だけのbefore_actionを見かけたときは、まずその必要性を疑ってみる、という習慣を付けた方がいいと思います。
まとめ
というわけで、この記事ではonlyの中身が1個だけのbefore_actionは定義しない方がいいよ、という話を書いてみました。
set_foo_barみたいなメソッドは必ずbefore_actionで呼び出さないといけない、と考えるのではなく、メリットとデメリットをちゃんと理解した上で使った方がいいケースと使わない方がいいケースを見極めましょう！