コントローラーを分けるとシンプルに記述できて良い感じだったので習慣にしたい

今回ユーザーの就職活動中かどうかのフラグをオンオフするというissueに取り組んでいて、その際にいただいたアドバイスについてここに残しておきたい。

条件としてはユーザー情報を更新するページと記事に対してコメントするページがあったとして、記事に対してコメントするページで就職活動中かどうかのフラグをオンオフするという実装が必要だった。

私は、以下のような感じで2種類のupdate処理を1つのコントローラー内に記述してレビュー依頼を行なった。

• user_controller(だいぶぼかしてある)

class UsersController < ApplicationController
  def update
    update_job_seek if only_job_seek?
    if @user.update(user_params)
      redirect_to users_url, notice: 'ユーザー情報を更新しました。'
    else
       ... # 何らかの処理
    end
  end

  def update_job_seek
    if @user.update(user_job_seek_params)
      ... # 何らかの処理
      return
    else
      ... # 何らかの処理
    end
  end

  private

  def user_params
    params.require(:user).permit(:name, :email, :age, :job_seek)
  end

  def user_job_seek_params
      ... # 何らかの処理
  end

  def only_job_seek?
    ... # job_seekというパラメータのみかをチェック
  end
end

このコードをレビューいただき、アドバイスとしてDHHはどのようにRailsのコントローラを書くのかという記事をもとに別のURLを作って更新処理を行なったほうが良いとうことだった。

この記事はだいぶ前であるが、教えていただいておりすっかり忘れてしまっていた。。。（今後はしっかり覚えておく！）

こちらの記事を読み、その通りに実践したところコントローラーの記述がシンプルになり、とても読みやすいコードになった。

• user_controller

class UsersController < ApplicationController
  def update
    if @user.update(user_params)
      redirect_to users_url, notice: 'ユーザー情報を更新しました。'
    else
       ... # 何らかの処理
    end
  end

  private

  def user_params
    params.require(:user).permit(:name, :email, :age, :job_seek)
  end
end

• job_seek_controller

class JobSeeksController < ApplicationController
  before_action :set_user, only: %i[update]

  def update
    if @user.update(user_params)
       ... # 何らかの処理
    else
       ... # 何らかの処理
    end
  end

  private

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

  def user_params
    params.require(:user).permit(:job_seeking)
  end
end

上記の通り、user_controllerでonly_job_seek?というメソッドを使って、job_seekというパラメータだけかどうかを判断していたが、コントローラーを2つに分けることでuser_controllerとjob_seek_controllerのそれぞれがすごくシンプルになった。

1つのコントローラーにたくさんの処理を任せれば任せるほどコードは読みにくくなるし、URLの設計の観点からもPUT /userでjob_seekだけをupdate するかどうかわからなくなる。

そのため、コントローラー分ける(責任を分ける)ことで、コードは読みやすく、URLの設計もPUT /job_seekでわかりやすくなる。

記事の引用の引用になるけれど、以下の言葉は確かに後の自分に怒りたくなるのだろうと思った。

自分の作ったコントローラの状態を悔やむのは決まって、作ったコントローラの数が少なすぎた時です。多くの処理を任せようとしすぎてしまうんです。

Full Stack Radioのインタビュー

ここで書いたからには、忘れないようにしたいし、コントローラーに変更を加える際にもう一度読み直したい。