コードレビューでよく指摘する事項をまとめている。
これらができるだけでも、初学者であれば他のエンジニアから一歩抜け出せると思う。
Rails固有の話も含まれている。
開発環境のみ使用するgemをGemfileでglobalに追加しない
たとえばhtml2slimは本番環境では不要。
developmentでしか使わない。
こういうgemはdevelopment groupに定義する
group :development do
gem "html2slim"
end
controllerのインスタンス変数はなるべく少なくする
controllerのインスタンス変数はなるべく少ないほうがよい。
ひとつのインスタンス変数を起点に取得できるならば、わざわざインスタンス変数に入れなおす必要はない。
class TweetsController < ApplicationController
def show
@tweet = Tweet.find(params[:id])
@user = @tweet.user
end
end
ユーザー名: <%= @user.name %>
@user = @tweet.userとする必要はなく、viewで@tweet.userを使えばよい。
class TweetsController < ApplicationController
def show
@tweet = Tweet.find(params[:id])
end
end
ユーザー名: <%= @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をつける。
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を定義する。
class User
validates :email, presence: true
validates :email, uniqueness: true
end
空配列を使わない
rubyであれば空配列を定義して、その中に要素をつめるような処理はrubyっぽくない。
rubyは配列操作のメソッドが豊富に用意されているので、injectやmapを使おう。
仕様になくてもvalidationをつける
仕様書は事細かくvalidationが定義されているわけではない。
仕様書になくても、
- 氏名のカナ入力欄であればカナ文字のみ許可する
- ウェブサイト入力欄であればウェブサイトのURLのフォーマットのみ許可する
などvalidationを付与しよう。
docker composeを使用しているのであればdotenv系のライブラリは不要
表題のとおり。docker composeのenv_fileを使おう。
他人のレコードを更新できてしまう
要件がユーザーがユーザー自身の情報を更新できるとする。
よくあるプロフィール編集画面だ。
以下のコードだと指定したidのユーザーを更新できてしまう。
多くの場合、AユーザーがBユーザーのプロフィールを更新できるのは意図しない動作だ。
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を取得すればよい。
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からユーザーを取得すればよい。
簡易な実装例は以下になる。
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を定義すればよい。
- resources :users
+ resource :user
上記のように定義すると/user/editのようにidをとらないroutingが生成される。
パスを文字列で指定している
文字列で指定すると間違ったパスや、存在しないパスを指定してもエラーにならずバグに気づきづらい。
<%= form_with url "hoge" %>
文字列ではなくpath helperを使おう
<%= form_with url hoge_path %>
find_byではnilを考慮するかそもそもfind_byを使わずfind_by!を使い例外を起こす
railsのfind_byはnilまたはレコードを返す。
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!で例外を起こしてしまったほうがよい。
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開発の基礎を身に着けてほしい。