レビューを受ける前に知っておきたい！Railsエンジニアが教えるレビューポイント

現役のフルスタックWebエンジニアで、プログラミングスクールの講師も務めている「とまだ」です。

プログラミングスクールで学んでいる方も、現場に出ている方も、日々コードレビューを受ける機会があると思います。

そして「コードレビューって、何を見られるのかな？」と不安に思うこともあるかもしれません。

今回は、私が現場で日々実践している、Railsプロジェクトにおけるコードレビューのポイントを詳しくお伝えします。

コードレビューの目的を理解しよう

コードレビューはバグの予防や品質向上に役立つ

「えっ、こんなバグ見逃してた！？」なんて経験、ありませんか？

コードレビューは、そういったバグを見逃さないための役割があります。

例えば、こんなコードがあったとします。

def calculate_total_price(items)
  total = 0
  items.each do |item|
    total += item.price * item.quantity
  end
  total
end

一見問題なさそうですが、実はここには潜在的なバグがあります。

item.priceやitem.quantityがnilの場合、エラーが発生してしまいます。

（意外と自分では気づかないものですよね...）

レビューでこういったケースを指摘し、以下のように改善できます。

def calculate_total_price(items)
  items.sum { |item| (item.price || 0) * (item.quantity || 0) }
end

このように、レビューを通じて潜在的な問題を早期に発見し、より堅牢なコードを書くことができるのです。

プロジェクト全体の可読性とメンテナンス性を高める

「このコード、書いた本人にしか分からないな...」」という経験はないでしょうか？

コードの可読性は、チームの生産性に直結します。

例えば、こんな長大なメソッドがあったとしましょう。

def process_order(order)
  user = order.user
  if user.age >= 20
    total = order.items.sum { |item| item.price * item.quantity }
    discount = total > 10000 ? total * 0.1 : 0
    order.update(total: total - discount)
    OrderMailer.confirmation(order).deliver_now
    redirect_to order, notice: 'Order was successfully created.'
  else
    render :new
  end

  # 以下、100行以上の処理...

このようなメソッドは、理解するのに時間がかかり、バグを見つけるのも大変です。

そのため、レビューを通じてメソッドを適切に分割し、可読性を向上させることが重要です。

では、具体的なレビューポイントを見ていきましょう。

可読性の観点

変数名やメソッド名が適切か

「名は体を表す」ということわざがありますが、プログラミングの世界でも同じです。

適切な命名は、コードの意図を明確に伝えます。

例えば、こんなコードがあったとします。

def x(a, b)
  a.map { |i| i * b }
end

このコードを見ただけでは、xやa、bが何を表しているのかが分かりません。

これを、以下のように変更することで、コードの意図が一目瞭然になります。

def multiply_array_elements(array, multiplier)
  array.map { |element| element * multiplier }
end

マジックナンバーやマジックストリングが使われていないか

マジックナンバーやマジックストリングは、コードの意図を不明瞭にし、将来的な変更を困難にします。

例：

if user.age >= 20
  # 処理
end

このコードを見た人が「20って何の意味だっけ？」となるのは避けたいですよね。

これを以下のように変更することで、コードの意図が明確になり、将来的な変更も容易になります。

LEGAL_ADULT_AGE = 20

if user.age >= LEGAL_ADULT_AGE
  # 処理
end

MVCアーキテクチャの観点

コントローラにビジネスロジックを詰め込みすぎていないか

「ファットコントローラ」は、Railsアプリケーションでよく見られる問題です。

例：

class OrdersController < ApplicationController
  def create
    @order = Order.new(order_params)
    if @order.save
      total = @order.items.sum { |item| item.price * item.quantity }
      discount = total > 10000 ? total * 0.1 : 0
      @order.update(total: total - discount)
      OrderMailer.confirmation(@order).deliver_now
      redirect_to @order, notice: 'Order was successfully created.'
    else
      render :new
    end
  end
end

上記のようなコントローラは、ビジネスロジックがコントローラに密結合しており、可読性やテスト性が低いです。

これを以下のように改善できます。

class OrdersController < ApplicationController
  def create
    @order = OrderService.create(order_params)
    if @order.persisted?
      OrderMailer.confirmation(@order).deliver_later
      redirect_to @order, notice: 'Order was successfully created.'
    else
      render :new
    end
  end
end

class OrderService
  def self.create(params)
    order = Order.new(params)
    if order.save
      order.calculate_total
      order.apply_discount
    end
    order
  end
end

class Order < ApplicationRecord
  def calculate_total
    update(total: items.sum { |item| item.price * item.quantity })
  end

  def apply_discount
    discount = total > 10000 ? total * 0.1 : 0
    update(total: total - discount)
  end
end

このように、ビジネスロジックを適切に分離することで、コントローラはシンプルになり、テストも書きやすくなります。

「ビジネスロジック」とは、アプリケーション固有のルールや計算、処理のことを指します。

パフォーマンスの観点

N+1クエリを防止できているか

N+1問題は、パフォーマンスに大きな影響を与える典型的な問題です。

例：

@posts = Post.all
@posts.each do |post|
  puts post.user.name
end

これは各投稿に対して個別にユーザー情報を取得するため、非効率です。

もう少し詳しく解説します。

今回の例では、PostモデルとUserモデルが1対1の関係にあると仮定します。

この場合、Post.allで全ての投稿を取得した後、@posts.eachで各投稿のユーザー情報を取得しています。

これにより、投稿数だけ追加のクエリが発行され、N+1問題が発生します。

例えるなら、投稿が100件あれば、ユーザー情報を取得するために101回のクエリが発行されることになります。

SELECT * FROM posts;
SELECT * FROM users WHERE id = 1;
SELECT * FROM users WHERE id = 2;
...

この問題を解決するためには、includesメソッドを使用します。

@posts = Post.includes(:user).all
@posts.each do |post|
  puts post.user.name
end

includesを使用することで、関連するユーザー情報を効率的に先読みし、パフォーマンスを大幅に向上させることができます。

テストコードの評価ポイント

Rails では RSpec や Minitest などのテスティングフレームワークを使ってテストコードを書くことが一般的です。

ここでは、テストコードの品質を評価するポイントを紹介します。

テストの品質：テストが適切な範囲をカバーしているか

単に「動作確認」程度のテストでは不十分です。

なぜならば、テストが通っていても、全てのケースを網羅していない可能性があるからです。

まず、悪い例を見てみましょう。

RSpec.describe Order, type: :model do
  describe '#apply_discount' do
    it '合計金額が10000円を超える場合、10%の割引が適用されること' do
      order = create(:order, total: 12000)
      order.apply_discount
      expect(order.total).to eq(10800)
    end
  end
end

確かに正常系のテストは書かれていますが、境界値や異常系のテストが欠けています。

「境界値」とは、ある値の前後の限界の値のことを指します。例えば、0や100などが境界値です。

「異常系」とは、通常の処理とは異なるケースのことを指します。例えば、ゼロ除算や存在しないレコードの取得などが該当します。

では、改善例を見てみましょう。

RSpec.describe Order, type: :model do
  describe '#apply_discount' do
    context '正常系' do
      it '合計金額が10000円を超える場合、10%の割引が適用されること' do
        order = create(:order, total: 12000)
        order.apply_discount
        expect(order.total).to eq(10800)
      end
    end

    context '境界値' do
      it '合計金額が10000円の場合、割引が適用されないこと' do
        order = create(:order, total: 10000)
        order.apply_discount
        expect(order.total).to eq(10000)
      end
    end

    context '異常系' do
      it '合計金額がマイナスの場合、割引が適用されないこと' do
        order = create(:order, total: -1000)
        order.apply_discount
        expect(order.total).to eq(-1000)
      end
    end
  end
end

このように、正常系だけでなく、境界値や異常系のケースも確実にテストすることで、予期せぬバグを防ぐことができます。

セキュリティ面

SQLインジェクションの対策をできているか

SQLインジェクションは非常に危険な脆弱性です。

例：

User.where("name = '#{params[:name]}'")

このようなクエリは、悪意のあるユーザーによって簡単に操作されてしまいます。

例えば、params[:name]に' OR '1' = '1'という文字列が渡されると、全てのユーザーが取得されてしまいますよね。

そのため、以下のように改善しましょう。

User.where(name: params[:name])

Railsのクエリビルダを使用することで、安全にSQLクエリを構築することができます。

CSRF対策が適切に行われているか

CSRF（Cross-Site Request Forgery）は、Webアプリケーションにおける一般的な脆弱性の1つです。

Railsでは、CSRF対策として、protect_from_forgeryがデフォルトで有効になっています。

class ApplicationController < ActionController::Base
  protect_from_forgery with: :exception
end

この設定により、Railsアプリケーションは自動的にCSRFトークンを生成し、リクエストに含めるようになります。

ただし、APIなどでCSRFトークンを利用できない場合は、適切な対策を講じる必要があります。

パラメータのバリデーションが適切に行われているか

ユーザーからの入力値は信用できません。

そのため、パラメータのバリデーションを適切に行うことが重要です。

例：

class UsersController < ApplicationController
  def create
    @user = User.new(user_params)
    if @user.save
      redirect_to @user
    else
      render :new
    end
  end

  private

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

逆に、以下のようにバリデーションを怠ると、セキュリティ上の脆弱性が生じます。

class UsersController < ApplicationController
  def create
    @user = User.new(params[:user])
    if @user.save
      redirect_to @user
    else
      render :new
    end
  end
end

例えば、params[:user]にadmin: trueという値が含まれていた場合、通常のユーザーが管理者権限を取得してしまう可能性があります。

そのため、必ずStrong Parametersを使用して、信頼できるパラメータのみを許可するようにしましょう。

最後に：レビューは前向きに受け取ろう

レビューは指摘をするという性質上、どうしても「自分のコードが否定された」と感じることがあります。

例えば、こんなフィードバックをもらったとします。

「このメソッドが長すぎるので、責務ごとに分割してはどうでしょうか？」

これを、「自分のコードが否定された」と受け取るのではなく、「より良いコードの書き方を学べるチャンス」と捉えましょう。

そして、「確かに長いですね。どのように分割するのがベストだと思いますか？」と、建設的な対話に発展させていくことが大切です。

ぜひ、この記事を参考にして、より良いコードを書くためのヒントを得てみてください！