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


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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

```ruby
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行以上の処理...
```

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

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

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

## 可読性の観点

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

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

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

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

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

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

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

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

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

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

例：

```ruby
if user.age >= 20
  # 処理
end
```

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

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

```ruby
LEGAL_ADULT_AGE = 20

if user.age >= LEGAL_ADULT_AGE
  # 処理
end
```

## MVC アーキテクチャの観点

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

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

例：

```ruby
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
```

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

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

```ruby
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
```

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

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

## パフォーマンスの観点

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

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

例：

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

```ruby
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
```

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

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

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

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

```ruby
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 インジェクションは非常に危険な脆弱性です。

例：

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

例：

```ruby
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
```

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

```ruby
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 を使用して、信頼できるパラメータのみを許可するようにしましょう。

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

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

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

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

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

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

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