繰り返されるインスタンス変数の初期化をメソッドに切り出す

前置き

作成中のTODOアプリでは、ユーザーがタスクを複数所有し、それぞれのタスクがサブタスクを複数所有しています。

アクションメソッド（数個だけ抜粋）

  def show
    @user = User.find(params[:user_id])
    @task = Task.find(params[:task_id])
    @subtask = Subtask.find(params[:id])
  end

  def update
    @user = User.find(params[:user_id])
    @task = Task.find(params[:task_id])    
    @subtask = Subtask.find(params[:id])
    
    if @subtask.update(subtask_params)
      redirect_to user_task_path(@user, @task, @subtask)
    end
  end
  
  def destroy
    @user = User.find(params[:user_id])
    @task = Task.find(params[:task_id])
    @subtask = Subtask.find(params[:id])
    
    @subtask.destroy
    redirect_to user_task_path(@user, @task)
  end

今回はこれらのコードをリファクタリングしていきます。

問題点

OnceAndOnlyOnce原則に反していることです。
各インスタンス変数の初期化が繰り返し記述されています。

対処

private
  def set_user
   @user = User.find(params[:user_id])
  end

このようなprivateメソッドを作ってbefore_actionで適用させれば、記述はこの一度で済ませることができます。

対処後

class SubtasksController < ApplicationController
 before_action :set_user
 before_action :set_task
 before_action :set_subtask

  def show
  end

  def update
    if @subtask.update(subtask_params)
      redirect_to user_task_path(@user, @task, @subtask)
    end
  end
  
  def destroy
   @subtask.destroy
    redirect_to user_task_path(@user, @task)
  end


  private
      def set_user
        @user = User.find(params[:user_id])
      end
      
      def set_task
        @task = Task.find(params[:task_id])
      end
      
      def set_subtask
        @subtask = Subtask.find(params[:id])
      end
end

ただこのset_○○には色々な考え方があり、

• 機能が増えていくにつれてbefore_actionが大量に増えてしまう

• before_actionの並び順によってエラーが起きる可能性もある

• アクションをパッと見た時にどのようなインスタンス変数を扱うのかがすぐ分からない

という問題もあるようです。今回のようにシンプルなアプリケーションであれば上記のやり方で良いと思います。
インスタンス変数の初期化の処理が複雑なのであれば、find_○○というメソッドを定義してそれぞれのアクションの中で使うと分かりやすくなります。

参考リンク

ソースコードの減らし方 - 基本的な考え方と10個の方法 - クラウドワークス エンジニアブログ

Rails の before_action :set_* って必要？ - ネットの海の片隅で