コードレビュー時に指摘されたこと #Rails

必要な機能の実装だけ行う

主にルーティングやcontrollerの記述

コメントアウトやconsole.logの削除

不要なものは削除する

view

表示したいrubyの記述は<%= %>にする
パスは可読性の観点からprefixパスで記述する rails routesで確認

Factorybotの記述

画像を添付するテストの場合は

spec/factories/items.rb

after(:build) do |item|
  item.image.attach(io: File.open("public/images/test_image.png"), filename: "test_image.png")
end

これを記述する。

ActiveHashを導入したときのテストコード

登録できない選択肢もテストする　（---とか）
選択肢はクォーテーションで囲まない

テストコード全般

数値型はシングルクォーテーションで囲まない
全角、半角、英字、数字のバリデーションをしている場合はそれ以外の文字列、混合の時のテストも記述する

モデル

バリデーションは要件定義に従って行う（画面は無視）
presence: trueをwith_optionsでまとめてあげる
has_one_attachedは1つのファイルを追加するために使用。そのため、単数系で記述。

controller

encrypted_passwordについてはdeviseにてデフォルトで扱われるためpermitに含める必要はない
destroyアクションにログインしているユーザーと出品者のidが同じだった場合の条件分岐を記述(理由としては、検証ツールから削除ボタンのhtmlを記述することで、削除する事ができてしまうため)

  def destroy
    if @item.user_id == current_user.id
      @item.destroy
      redirect_to root_path
    end
  end

privateメソッドを上手に使う

app/controllers/items_controller.rb

  private

  def set_item
    @item = Item.find(params[:id])
  end

  def item_params
    params.require(:item).permit(:name, :info, :category_id, :status_id, :fee_status_id, :prefecture_id, :schedule_delivery_id,
                                 :price, :image).merge(user_id: current_user.id)
  end

  def move_to_index
    if @item.user_id != current_user.id
      redirect_to action: :index
    end
  end

set_itemでItem情報をぶち込む記述をしています。
これにbefore_actionメソッドを組み合わせることで、わざわざ、showやdestroyなど個別の投稿をfindする記述は不要になります。

さらに、move_to_indexメソッドは投稿者とログイン中のユーザーが一致しない時はindexメソッドを実行する、というメソッドなので、先ほどのdestroyやeditなどはこのbefore_actionを使用すれば、わざわざメソッド内に条件分岐を記述する必要はなくなります。

不正アクセス防止のため、editだけでなく、updateアクションもログインしているユーザーと出品者のidが同じだった場合の条件分岐を記述（上述のとおり、privateメソッドとbefore_actionで記述すればOK）