コードレビューでよく指摘する内容（これができれば頭一つ抜けることができる！）

コードレビューでよく指摘する事項をまとめている。
これらができるだけでも、初学者であれば他のエンジニアから一歩抜け出せると思う。
Rails固有の話も含まれている。

## 開発環境のみ使用するgemをGemfileでglobalに追加しない

たとえば`html2slim`は本番環境では不要。
developmentでしか使わない。
こういうgemはdevelopment groupに定義する

```ruby:Gemfile
group :development do
  gem "html2slim"
end
```

https://github.com/slim-template/html2slim

## controllerのインスタンス変数はなるべく少なくする

controllerのインスタンス変数はなるべく少ないほうがよい。
ひとつのインスタンス変数を起点に取得できるならば、わざわざインスタンス変数に入れなおす必要はない。

```ruby:app/controllers/tweets_controller.rb

class TweetsController < ApplicationController
  def show
    @tweet = Tweet.find(params[:id])
    @user = @tweet.user
  end
end
```

```erb:app/views/tweets/show.html.erb
ユーザー名: <%= @user.name %>
```

`@user = @tweet.user`とする必要はなく、viewで`@tweet.user`を使えばよい。

```ruby:app/controllers/tweets_controller.rb

class TweetsController < ApplicationController
  def show
    @tweet = Tweet.find(params[:id])
  end
end
```

```erb:app/views/tweets/show.html.erb
ユーザー名: <%= @tweet.user.name %>
```

## controllerだけで使う値はインスタンス変数に入れる必要はない

Railsではcontrollerでインスタンス変数にいれることでviewで使用可能になる。
逆にいうとviewで使用しない値はインスタンス変数にいれる必要はない。
ローカル変数で十分だ。
これは適切な例がパッと思い浮かばなかったのであとで追記しておく。

## pull requestの対応内容に不要な修正を含めない

pull requestの対応範囲が「tweetの詳細画面実装」なのに、まったく関係ないファイルの修正や
「tweetの編集機能」が実装されていたりする。
まったく関係ないファイルの変更は含めない、含めるとしたらコメントで補足するようにしよう。
全く関係のない機能実装は別のpull requestにわけよう。

## 不要なファイルは削除する

railsのgeneratorでcontrollerを作成するとたとえばcreate.html.erbやdestroy.html.erbも作成される。
これらはほとんどの場合不要である。
使用していないファイルは潔く削除する。
「後から使うかも？」などと考えて残しておく必要はない。
ほとんどの場合使わずにゴミファイルとして残り続けるので、使うときに追加すればよい。

## db制約がない

必須入力のカラムはNOT NULLにする。
railsであればmigrationファイルで`null: false`をつける。

```ruby
class CreateItems < ActiveRecord::Migration[7.0]
  def change
    create_table :items do |t|
      t.string :name_badge, null: false
    end
  end
end
```

たとえばuserのemailなど一意である必要がある場合はUNIQUE INDEXをつける。
これでapplicationで間違って一意でないemailを登録しようとしてもdbでエラーを起こしてくれる。

## dbにそろえてvalidationをつける

dbでNOT NULLやUNIQUE INDEXを付与したら、applicationでもvalidationを付与する。
railsであればmodelのvalidationを定義する。

```ruby:app/models/user.rb

class User
  validates :email, presence: true
  validates :email, uniqueness: true
end
```

## 空配列を使わない

rubyであれば空配列を定義して、その中に要素をつめるような処理はrubyっぽくない。
rubyは配列操作のメソッドが豊富に用意されているので、injectやmapを使おう。

https://techracho.bpsinc.jp/hachi8833/2020_11_06/59639

## 仕様になくてもvalidationをつける

仕様書は事細かくvalidationが定義されているわけではない。
仕様書になくても、
- 氏名のカナ入力欄であればカナ文字のみ許可する
- ウェブサイト入力欄であればウェブサイトのURLのフォーマットのみ許可する

などvalidationを付与しよう。

## docker composeを使用しているのであればdotenv系のライブラリは不要

表題のとおり。docker composeのenv_fileを使おう。

## 他人のレコードを更新できてしまう

要件がユーザーがユーザー自身の情報を更新できるとする。
よくあるプロフィール編集画面だ。
以下のコードだと指定したidのユーザーを更新できてしまう。
多くの場合、AユーザーがBユーザーのプロフィールを更新できるのは意図しない動作だ。

```ruby
class UsersController < ApplicationController
  def update
    @user = User.find(params[:id])
    if @user.update(user_params)
      redirect_to @user, notice: "ユーザーの更新に成功しました"
    else
      flash.now.alert = "ユーザーの更新に失敗しました"
      render :edit, status: :unprocessable_entity
  end 
end
```

この場合はログインユーザーが自分自身のみ編集できるようにするとよい。
deviseならcurrent_userを取得すればよい。

```ruby
class UsersController < ApplicationController
  before_action :authenticate_user!

  def update
    @user = current_user
    if @user.update(user_params)
      redirect_to @user, notice: "ユーザーの更新に成功しました"
    else
      flash.now.alert = "ユーザーの更新に失敗しました"
      render :edit, status: :unprocessable_entity
  end 
end
```

もし自前でログイン認証を実装している場合はsessionからユーザーを取得すればよい。
簡易な実装例は以下になる。

```ruby
class UsersController < ApplicationController
  def update
    @user = current_user
    if @user.update(user_params)
      redirect_to @user, notice: "ユーザーの更新に成功しました"
    else
      flash.now.alert = "ユーザーの更新に失敗しました"
      render :edit, status: :unprocessable_entity
  end 

  private

  def current_user
    @user ||= User.find(session[:user_id])
  end
end
```

上記のように実装するとルーティングでuserのidは不要になる。

そのためresourcesではなく、resourceを定義すればよい。

```diff
- resources :users
+ resource :user
```

https://railsguides.jp/routing.html#%E5%8D%98%E6%95%B0%E5%BD%A2%E3%83%AA%E3%82%BD%E3%83%BC%E3%82%B9

上記のように定義すると`/user/edit`のようにidをとらないroutingが生成される。

## パスを文字列で指定している

文字列で指定すると間違ったパスや、存在しないパスを指定してもエラーにならずバグに気づきづらい。

```erb
<%= form_with url "hoge" %>
```

文字列ではなくpath helperを使おう

```erb
<%= form_with url hoge_path %>
```

https://railsguides.jp/routing.html#%E3%83%91%E3%82%B9%E3%81%A8url%E7%94%A8%E3%83%98%E3%83%AB%E3%83%91%E3%83%BC

## find_byではnilを考慮するかそもそもfind_byを使わずfind_by!を使い例外を起こす

railsのfind_byはnilまたはレコードを返す。

```ruby
  def destroy
    tweet = Tweet.find(params[:tweet_id])
    favorite = current_user.favorites.find_by(tweet_id: tweet.id)
    favorite.destroy!
  end
```

つまり↑のコードはfind_byでnilが返った場合に`favorite.destroy!`でnilエラーが発生する。
上記の場合は削除したいfavoritesが見つからなければfind_by!で例外を起こしてしまったほうがよい。

```ruby
  def destroy
    tweet = Tweet.find(params[:tweet_id])
    favorite = current_user.favorites.find_by!(tweet_id: tweet.id)
    favorite.destroy!
  end
```

もしレコードが見つからないことが十分にありえる場合は、find_byを使いつつnilをケアしたコードを書こう。

## Railsのレールから外れている

せっかくRailsを使っているのにレールから外れている。
守破離はレールに乗った開発の基礎を身に着けてから。

まずはRailsガイドの以下のページでRailsでCRUD開発の基礎を身に着けてほしい。

https://railsguides.jp/getting_started.html