🎼

別のメソッド内で定義したインスタンス変数を使うな高校校歌

2025/01/23に公開

初めに結論

別のメソッド内で定義したインスタンス変数を使うな

問題提起

複数箇所で再利用したい処理や、あるメソッドの可読性を低下させるような長い処理を別のメソッドに抽出したい時ってあると思います。
しかし、これはあまり良くないメソッドの抽出だなと思うケースがあったのでメモ。

class HogesController < ApplicationController

  def show
    set_hoge

    response = {
      hoge: @hoge,
    }
    render json: response, status: :ok
  end

  def create
    # 省略
  end

  # その他いろいろなメソッド

  private

  # いろいろ

  def set_hoge
    @hoge = Hoge.find(params[:id])
  end

  # いろいろ
end

サンプルコードにおけるHogesControllershowアクションを見ると、
レスポンスの一部にどこで定義されたのか分からない@hogeが使われています。
で、コード全体を見てみると、set_hogeメソッド内で定義されていたんだねってことが分かります。

エディタの検索機能を使えば現状問題ないことは分かりますが、
もし今後、機能の追加やリファクタリングをする中で、set_hogeの呼び出し位置とresponseの定義位置の順番が逆転してしまったら終わりです。

対応

サンプルコードの例だと、以下のように修正しておくと
以後の変更の際にデグレが起こりにくいと思います。

  def show
    @hoge = fetch_hoge

    response = {
      hoge: @hoge,
    }
    render json: response, status: :ok
  end

  private

  def fetch_hoge
    Hoge.find(params[:id])
  end

嗚呼〜

嗚呼〜別のメソッド内で定義したインスタンス変数を使うな高校〜

Discussion