コードレビューで気をつけるべきこと

最近コードレビューをする機会が増えてきたので、自分なりに意識していることを備忘録としてまとめておきます。

まず良いところを見つける

指摘ばかりになると相手も萎縮してしまうので、本当に良いと思った部分は素直に伝えるようにしています。「この抽象化わかりやすいですね」とか「エラーハンドリング丁寧で助かります」みたいな一言があるだけで、レビュー全体の雰囲気が変わる気がします。

たとえば、こういうコードを見たとき

class User < ApplicationRecord
  validates :email, presence: true, format: { with: URI::MailTo::EMAIL_REGEXP }
  
  before_save :normalize_email
  
  private
  
  def normalize_email
    self.email = email.downcase.strip if email.present?
  end
end

「before_saveで正規化してくれているの、データの整合性が保たれていいですね。present?のチェックも入っていて安全です」みたいに、良い点を先に伝えると、その後の改善提案も受け入れてもらいやすくなります。

いきなり指摘せず、理由を聞く

「ここはこうした方がいいです」って言う前に、「なぜこの実装を選んだんですか?」と聞くようにしています。自分が想定していなかった制約や要件があることも多くて、結果的に現在の実装が最適解だったりすることもあります。

例えば、こういうコードがあったとして

class OrdersController < ApplicationController
  def create
    order = Order.create!(order_params)
    OrderMailer.confirmation_email(order).deliver_now
    redirect_to order_path(order)
  end
end

「メール送信を同期的に行っているのが気になりますが、何か理由がありますか?」と聞くと、「実はこのメールにはPDF添付があって、非同期だとファイル生成のタイミングで問題が出るんです」みたいな背景が分かったりします。それなら納得できますし、「Sidekiqのperformメソッド内でPDF生成する方法もありますが、検討されましたか?」みたいに別の提案もできます。

優先度を明示する

レビューコメントには優先度をつけるようにしています。

• マストで直してほしいもの（バグやセキュリティリスク）
• できれば対応してほしいもの（設計や可読性の改善）
• 好みレベルの提案（nit: をつけて）

# ❌ これはマスト修正
@users = User.where("name = '#{params[:name]}'")
# SQLインジェクションのリスクがあります。プレースホルダーを使ってください

# ⚠️ できれば改善したい
def process(data)  # dataが何のデータか不明瞭です
  # ...
end

# 💭 好みの問題
nit: `unless`より`if !`の方が読みやすいかもしれません（チームの好み次第ですが）

全部同じ温度感で書くと、何から手をつければいいのか分からなくなってしまうので。

エッジケースを想像する

nilが渡されたら、空の配列だったら、想定外の値が来たら…という視点で見るようにしています。

class User < ApplicationRecord
  def full_name
    "#{first_name} #{last_name}"
  end
end

このコードを見たら、「first_nameやlast_nameがnilの場合の挙動、確認させてください」とコメントします。

class User < ApplicationRecord
  def full_name
    [first_name, last_name].compact.join(' ')
  end
end

こういう防御的なコードになっているか、それともバリデーションで保証されているのか、確認するようにしています。

変数名の具体性

data、result、tmpみたいな汎用的な名前が続くと、後で読み返したときに何を表しているのか分からなくなります。

# あまり良くない例
def create
  data = User.find(params[:id])
  result = process(data)
  render json: result
end

# 改善例
def create
  user = User.find(params[:id])
  activated_user = process(user)
  render json: activated_user
end

「resultよりactivated_userの方が、何が入っているか分かりやすいかもしれません」みたいに提案することが多いです。

Railsの規約に沿う

Railsには「設定より規約」という思想があるので、標準的なパターンから外れる場合は理由を確認します。

# 気になる例
class User < ApplicationRecord
  def self.get_active_users
    where(status: 'active')
  end
end

「スコープとして定義した方がRailsらしいかもしれません」とコメントします。

class User < ApplicationRecord
  scope :active, -> { where(status: 'active') }
end

ただ、複雑なロジックの場合はクラスメソッドの方が適切なこともあるので、一概には言えないですが。

コントローラーは薄く、モデルに寄せる

Fat Modelの是非はありますが、少なくともコントローラーにビジネスロジックを書くのは避けたいです。

# あまり良くない例
class OrdersController < ApplicationController
  def create
    @order = Order.new(order_params)
    @order.total = @order.items.sum(&:price)
    if @order.user.premium?
      @order.total *= 0.8
    end
    @order.save
  end
end

「この計算ロジック、Orderモデルに移動した方がテストしやすいかもしれません」と提案します。

class Order < ApplicationRecord
  before_save :calculate_total

  private

  def calculate_total
    self.total = items.sum(&:price)
    apply_discount if user.premium?
  end

  def apply_discount
    self.total *= 0.8
  end
end

N+1問題のチェック

これはRailsあるあるなので、必ず確認します。

# 問題がある例
def index
  @articles = Article.published.where('view_count > ?', 100)
end

/ View
- @articles.each do |article|
  .article-card
    h2 = article.title
    .author
      = article.author.name
      = image_tag article.author.avatar_url
    .stats
      span = "#{article.comments.size}件のコメント"

「ここN+1が発生していますね」とコメントして、修正を提案します。

このケースでは、whereで絞り込みをしているのでincludesではなくeager_loadを使った方が良さそうです。

# includesだと2回SQLが発行される
def index
  # SELECT * FROM articles WHERE published = true AND view_count > 100
  # SELECT * FROM authors WHERE id IN (...)
  # SELECT * FROM comments WHERE article_id IN (...)
  @articles = Article.published
                     .where('view_count > ?', 100)
                     .includes(:author, :comments)
end

# eager_loadだとJOINで1回のSQLになる
def index
  # SELECT articles.*, authors.*, comments.* 
  # FROM articles 
  # LEFT OUTER JOIN authors ON authors.id = articles.author_id
  # LEFT OUTER JOIN comments ON comments.article_id = articles.id
  # WHERE articles.published = true AND articles.view_count > 100
  @articles = Article.published
                     .where('view_count > ?', 100)
                     .eager_load(:author, :comments)
end

特に、関連テーブルのカラムで絞り込みたい場合はeager_loadが必須です。

ruby

# これは動かない（authorテーブルがJOINされていないため）
Article.includes(:author).where('authors.active = ?', true)

# これなら動く
Article.eager_load(:author).where('authors.active = ?', true)

セキュリティ視点を持つ

Railsは多くのセキュリティ対策が組み込まれていますが、それでも確認すべき点はあります。

# 危険な例
class UsersController < ApplicationController
  def update
    @user = User.find(params[:id])
    @user.update(params[:user])
  end
end

「Strong Parametersを使っていないので、マスアサインメント脆弱性がありそうです」とコメントします。

def update
  @user = User.find(params[:id])
  @user.update(user_params)
end

private

def user_params
  params.require(:user).permit(:name, :email)
end

あと、rawやhtml_safeを使っている箇所は特に注意して見ます。

/ XSSのリスク
== @user.comment

/ 代わりに
= sanitize @user.comment

トランザクションの必要性

複数のレコード操作がある場合、トランザクションで囲まれているか確認します。

# 問題がある例
def transfer_money(from_user, to_user, amount)
  from_user.update(balance: from_user.balance - amount)
  to_user.update(balance: to_user.balance + amount)
end

「途中で失敗した場合、データの整合性が崩れそうです。トランザクションで囲んだ方がいいかもしれません」とコメントします。

def transfer_money(from_user, to_user, amount)
  ActiveRecord::Base.transaction do
    from_user.update!(balance: from_user.balance - amount)
    to_user.update!(balance: to_user.balance + amount)
  end
end

コールバックの乱用に注意

before_saveやafter_createは便利ですが、暗黙的な動作が増えすぎると追いづらくなります。

# 気になる例
class User < ApplicationRecord
  after_create :send_welcome_email
  after_create :create_default_settings
  after_create :notify_admin
  after_create :update_analytics
  # ...たくさんのコールバック
end

「コールバックが多いと、Userを作成したときの挙動が追いづらくなりそうです。Service Objectに切り出すのはどうでしょうか」と提案することがあります。

パフォーマンスは状況次第

明らかな問題は指摘しますが、細かい最適化については慎重に判断しています。

# これは指摘する
User.all.each do |user|
  user.posts.update_all(published: true)
end

# バッチ処理にした方がいいです
User.find_each do |user|
  user.posts.update_all(published: true)
end

一方で、こういう最適化は状況次第：

# 可読性重視
users.map(&:name).join(', ')

# vs パフォーマンス重視（ただし配列が小さければ差はない）
users.pluck(:name).join(', ')

実際にボトルネックになっていないなら、可読性を優先することもあります。

最近気づいたこと

感情的になっているときは、レビューコメントを書いた後に少し時間を置いてから送信するようにしています。

❌ 「これ、Railsの基本ですよね」
⭕ 「Railsのガイドでは○○が推奨されているので、参考にしてみてください」

「こうした方がいいです」という断定より、「こういう方法もありますが、いかがでしょうか」という提案の方が受け入れられやすいです。

あと、自分がレビューされる側に回ったときの感想を覚えておくと参考になります。嫌だった指摘の仕方、嬉しかったフィードバックの形式など。

終わりに

コードレビューは品質を上げるだけでなく、チーム内で知識を共有する貴重な機会だと感じています。レビューする側もされる側も学びがあるような、建設的なやり取りを心がけていきたいです。