レビューしてもらう際に気をつけた方がいいことをメモしておく
each
- mapで代用できないか考えてみる
- mapよりも、each_with_objectやinjectの方が良いか考えてみる
- trueやfalseの判定であれば、all?やnone?、any?、one?が使えないかを考えてみる
- ハッシュから特定のキーを取得するのであれば
slice、削除するのであればexceptが使える
unless
sample.rb
# ifで書いた方が可読性が高いので置き換えられないかを考えてみる
unless a == b
↓
if a != b
# 条件が複数あった場合は、ifを使った方が可読性は高いので使わないようにする
unless a.present? && b.present?
↓
if a.blank? || b.blank?
# unless~else は見通しが悪いので使わないようにする
unless user
...
else
...
end
↓
if user.blank?
...
else
...
end
トランザクション
- 一連のロジックで、複数の更新処理がある場合は利用
- ロールバックされるように、例外が出るようになっているか（ex.
save!やupdate!) - そもそも
ActiveRecord::Base.transactionで括らずに、saveやupdate一発でまとめられないかを検討
破壊的メソッドを多用しない
意図せず破壊的メソッドが呼ばれることによって、バグ含むコードを生む可能性を下げるため
hash = { hoge: 111 }
hash.merge!(hoge2: 222)
とするよりも
hash = hash.merge(hoge2: 222)
バッチ処理
- 大量のレコード数を扱うことが多いので、オブジェクトの生成コストを考慮して、
find_eachを利用する
同じロジックをまとめることができないか
- 同じクラス内であればprivate・インスタンス・クラスメソッド化するか
scope使えないか検討する - 別クラスであれば
concernにまとめる - 継承元が同じであれば、
継承元にまとめる(ex. application_controllerなど) - view側の表示ロジックであれば、
helperやdecoratorの利用を検討してみる。または、partialとして切り出せるか検討する。 - validationであれば、独自バリデーターを作る
migrationファイル
- 必須項目であればnull: falseは入っているか
- index必要か
- uniq制約必要か
- マイグレーション実行後にロールバックできるか
- varcharでいいのか(charにした方が良いか)
- カラムの位置の調整が必要であればafterオプションなどで調整する
モデル
- migrationファイルの定義と同じvalidationが入っているか
- spec書いているか
- DBに保存するデータなのかを考える。定数として定義したり、config、またはredisなどを使えるかどうか検討してみる
- ActiveRecordが必要なのか？ActiveHashで代用できるか考えてみる
- rails consoleでメソッドの動きを確認してみる
ビュー
- キャッシュが必要か？フラグメントキャッシュやredisにキャッシュさせるなど検討してみる
- 共通化するためにパーシャル利用したほうがいいか
- メタタグに必要な情報入っているか、OPGを忘れてないか
- アクセス解析しているのであれば、パラメーター付け忘れてないか
- パーシャルに渡している引数の取扱いは注意する。引数を削除したことにより、他の場所で参照できなくなり落ちることがあるため。
local_assignsを使うようにすることを検討する。
WEBrickの利用
- pryを使ってデバッグしてバグを減らす
- ログを確認して動きを確認する
その他
- rubocopを導入して、細かい指摘を受けないようにする（不要なスペースがあったり、チームのコーディングルールに即していないとか、指摘する方も面倒になるので）
- gemを入れる時は活動状況を確認してみる。（バグった時に質問できない、解決できない負債を減らすため）
- jsを修正した祭にはchromeのconsoleでエラーが出てないかチェックする
- Gemfile.lockの末端のbundlerのバージョンは各環境で統一されているか確認する