Railsコードレビューしていて気になったことメモ

takeyuweb

同じロジックを複数の箇所で書かないようにしたい
「TaskTemplate から Task を作成する」ロジックが次の2箇所に出現している

手動で TaskTemplate から Task を作成
ある条件で TaskTemplate から自動で Task を生成

たとえば将来 Task の属性が増えて、対応する属性を TaskTemplate に追加したとき、TaskTemplageとTaskの属性の対応付けを追加する必要がありますが、変更箇所が複数になってしまいます。

「TaskTemplate から Task を作成する」例えば次のようなメソッドにまとめる

TaskTemplate#build_task(task_attributes)

takeyuweb

Resourceでないルーティングはなるべく避ける

こういう（Rails標準でない）名前付きroutesが増えると、routingが読みづらくなるので

get 'tasks/finish/:id', controller: 'tasks', action: 'finish', as: :finish_task
get 'tasks/unfinish/:id', controller: 'tasks', action: 'unfinish', as: :unfinish_task

可能な限りresource(s)で書きたいところ

resources :tasks, except: %i[new] do
  resource :finish, only: %i[create destroy], module: :tasks
end

class Tasks::FinishesController < ApplicationController
  def create
    Task.find(params[:task_id]).finish!
  end

  def destroy
    Task.find(params[:task_id]).unfinish!
  end
end

takeyuweb

データベース等への変更があるものは、 get を使わないようにしましょう

CDNやプロキシキャッシュなどでキャッシュしたいとき、キャッシュしてよいのかどうかで困りますし、DatabaseSelector middlewareのようなものを使うときにも困ります。

takeyuweb

1つのアクションに複数の意味を持たせるのは控えるべき
たとえば TasksController#indexで

/tasks タスク一覧画面。
/tasks?user_id=1 User1のタスク一覧画面。

class TasksController < ApplicationController
  def index
    @tasks = Task.all
    @tasks = @tasks.where(user_id: params[:user_id])  if params[:user_id]
  end
end

「タスク一覧画面の検索条件として user_id を与える」であればこれで問題ないと思います。

ですが、特定のUserのタスク一覧を別の画面として提供するのであれば、ここで行うべきではないと考えます。ビューに params[:user_id] での分岐が入るなどロジックが入ってしまいがちです。

resources :tasks
resources :users do
  resources :tasks, module: :users
end

class Users::TasksController < ApplicationController
  def index
    @user = User.find(params[:user_id])
    @tasks = user.tasks
  end
end

takeyuweb

作成と編集でフォーム部分のテンプレートを部分テンプレートとして共有する場合、作成と編集でフォーム項目が異なるケースもあるので、FormBuilderオブジェクトを渡す設計の方が扱いやすい

new.html.erb

<%= form_with model: @post do |form| %>
  <%= render partial: "form", locals: { form: } %>
  <%= form.submit "Create" %>
<% end %>

edit.html.erb

<%= form_with model: @post do |form| %>
  <%= render partial: "form", locals: { form: } %>
  <label>
    <%= form.check_box :notify %>
    更新を通知する
  </label>
  <%= form.submit "Create" %>
<% end %>

部分テンプレート内で FormBuilder を作っていると、このようなケースに対応できない

takeyuweb

trait なしの Factory.build(:post) で作成されるインスタンスは valid? #=> true になるようにする
invalidなインスタンスを作りたい場合は、そのためのtraitを作るか、属性を渡して作る

# こういうのはNG
# trait でなくとも valid であるべき
trait :valid do
  title { "xxxxx" }
end

takeyuweb

部分テンプレートでインスタンス変数は使わないほうがよい

_comment.html.erb

<%=simple_format comment.body %>
（<%=link_to "View Post", @post %>）

部分テンプレートにインスタンス変数を含むと、render partial: "comment", locals: { comment: } を書いたテンプレートにインスタンス変数が存在することを要求するが、これは部分テンプレートの関心事ではない

_comment.html.erb

<%=simple_format comment.body %>
（<%=link_to "View Post", comment.post %>）

takeyuweb

has many なインスタンスを作る時はアソシエーションのメソッドを使います

@comment = Comment.new(**comment_params, post: @post)

ではなく

@comment = @post.comments.build(comment_params)
# または
@comment = @post.comments.create(comment_params)

を使いましょう。

@post.comments にキャッシュさせないとバリデーションエラー時のフォーム再表示時に問題が起こったり、@post の更新と同時に @comment も作成/更新したい場合にうまくいきません。

@post.transaction do
  @post.comments.build(comment_params)
  @post.update!(post_params)  # post の更新と comment の作成 が行われる
end

takeyuweb

Controllerにバリデーションに相当するロジックを書かないようにしましょう

takeyuweb

DB制約の NOT NULL を付けたカラムは、フォームでのエラー表示のために、モデルバリデーションを付けてください。
未入力のままフォーム送信した際に null violation error が発生してしまいます。

validates :status, presence: true

ただしbooleanの場合は

validates :done, inclusion: { in: [true, false] }