Open2024/05/17にコメント追加4

Rails アプリケーションのレビューで指摘するヤツ

コントローラでのインスタンス変数の利用は必要最小限に留める

指摘するケース

# HogeController のソースコード

  before_action :set_current_category, only: :show

  def show
     render fuga
  end

  (略)

  private

  def set_current_category
    @current_category = Category.find(params[:category_id])
  end

  # このメソッドは、事前に `set_current_category` メソッドが呼び出されているべきである。
  # という暗黙の制約を持っている。
  def fuga
   # ... (なんらかの処理) ...

    # インスタンス変数が設定されている前提で、その `hoge` の値を利用してなんらかの
    # 処理 ( calculate ) を実施する
    calculate @current_category.hoge
  end

問題

メソッド間に暗黙の制約 ( 依存関係 ) が生じている
暗黙の制約が増えると、コードを読む人の脳内のワーキングメモリを消費する
そのため、このようなコードが増えると考慮漏れなどからバグを誘発しやすくなり、メンテナンスが困難になる

snaka

修正案

こうしたい

  def show
-     render fuga
+     render fuga(@current_category.hoge)
  end

-  def fuga
+  def fuga(hoge)
   # ... (なんらかの処理) ...

    # 引数で渡された `hoge` を使って処理 ( calculate ) を実施する
-    calculate @current_category.hoge
+    calculate hoge
  end

snaka

指摘の背景

コントローラのコードは基本的に「状態」を持たない方が良い
「状態」を持たないことでコードが読みやすくなり、バグが混入する確率を低減することができる

(コントローラの内部処理に関する言及)

もちろん内部では他にもいろいろやっているのは承知のうえで、概念上は以下にまとめられます。

インスタンスを1つ作成する

メソッドを1つ呼び出す

作成されたオブジェクトを渡す

これは本質的にオブジェクト指向的というより関数型的です。

snaka

個人的には、 set_current_category のような Callback でインスタンス変数をセットするパターンもあまり乱用したくない。しかし、Rails way として一般的に使われてしまっているので許容する。