現役のフルスタック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を使用して、信頼できるパラメータのみを許可するようにしましょう。
最後に：レビューは前向きに受け取ろう
レビューは指摘をするという性質上、どうしても「自分のコードが否定された」と感じることがあります。
例えば、こんなフィードバックをもらったとします。
「このメソッドが長すぎるので、責務ごとに分割してはどうでしょうか？」
これを、「自分のコードが否定された」と受け取るのではなく、「より良いコードの書き方を学べるチャンス」と捉えましょう。
そして、「確かに長いですね。どのように分割するのがベストだと思いますか？」と、建設的な対話に発展させていくことが大切です。
ぜひ、この記事を参考にして、より良いコードを書くためのヒントを得てみてください！