伊藤淳一が考える「コードレビューの観点とアプローチ」

はじめに

前編の記事では、コードレビュー概論として「コードレビューの目的」と「コードレビューの進め方」を説明しました。

この後編では、より具体的・実践的なトピックとして「コードレビュー時に着目すべきポイント」と「コードレビュー時に役立つテクニックや考え方」を説明します。

本記事の対象読者

後編も前編と同じで、以下のような読者を想定して書かれています。

• コードレビューされる側から、コードレビューする側に最近回った1〜2年目の新人エンジニア

つまり、これまでコードレビューを受けることは何度かあったけど、コードレビューする側に回るのは初めてで、どうコードレビューすればいいか迷っている、という若手を対象にしています。

また、開発チームで新人エンジニアを指導しているリードエンジニアのみなさんも、チーム内で「コードレビューの手順や観点」を議論するための叩き台として本記事の内容をご活用ください。

コードレビュー時に着目すべきポイント

最初に説明するのは「コードレビュー時に着目すべきポイント」です。

「このプルリクエスト（以下PR）をコードレビューしてね」と言われても、経験が浅いエンジニアは「いったい何をどうチェックしたらいいんだ」と困ってしまうのではないでしょうか？

先輩エンジニアはきっと何かしらの着眼点を持ってコードレビューしているはずですが、わざわざその着眼点を説明してもらった人は少ないと思います。

そこで、ここでは筆者が培ってきた長年の経験をベースとして、「可読性、保守性、堅牢性、正しさ、セキュリティ、パフォーマンス」という6つのテーマに分けて、コードレビュー時の観点を説明します。

おことわり

可能ならありとあらゆる観点をみなさんにお伝えしたいのですが、そうすると本が一冊書けるぐらいのボリュームになってしまいます。そのため、本記事では各テーマについて、とっかかりになりそうなポイントを3つずつ紹介します。

つまり、本記事の目的はコードレビューの観点を全網羅することではなく、「なるほど、先輩はコードレビューするときにそういうところを見ているのか」という気づきをみなさんに提供することです。

なお、本記事の最後には、良いコードを学ぶのに役立つ書籍や情報源を紹介しています。これらの書籍を読むことで「自分の中の審美眼」をさらに養うことができるはずです。

また、筆者は普段Ruby on Railsを使ってWebアプリケーションを開発しているため、この記事で紹介するサンプルコードはRubyやRuby on Railsを前提としたものになっています。ですが、大半の内容は他の言語や他のフレームワークでも活用できると考えています。

それでは本編に進みましょう。

可読性の観点

もっとシンプルに書けないか

同じことを実現するのに、より短く、よりシンプルに書ける方法があるなら、それを採用する。たとえば、標準ライブラリでできることを自前で実装している場合など。

ただし、短くしすぎて可読性を損なうのはNG。あくまで「シンプルで読みやすいコード」を目指す。

# NG: こう書かずに、
user_names = []
users.each do |user|
  user_names << user.name
end

# OK: こう書く
user_names = users.map(&:name)

わかりにくい変数名やメソッド名を付けていないか

わかりにくい名前や誤解を招くような名前はNG。ほどよく具体的で、ほどよく抽象的な名前を付ける。名前重要。

# NG: ブログにファイルをアップロードするの？？（動詞＋目的語）
files = blog.upload_files

# OK: あー、ブログにアップロードされたファイルを取得するのか！（過去分詞＋名詞）
files = blog.uploaded_files

無理に長いコードを1行で書こうとしていないか

コンピュータは理解（実行）できても、人間がぱっと理解できないコードはNG。

<!-- NG: 1行が長すぎて、ぱっと理解しづらい -->
<%= f.select :role, company.products.on_sale.map { |p| [p.name, p.id] }, {}, class: "form-select-box" %>

<!-- OK: 一度変数に入れた方が理解しやすい -->
<% choices = company.products.on_sale.map { |p| [p.name, p.id] } %>
<%= f.select :role, choices, {}, class: "form-select-box" %>

保守性の観点

責務をきちんと意識しているか

「とりあえず動かすだけ」なら、どこに書いても同じように動くが、適当な場所に書いてしまうと再利用性が低下したり、変更に弱くなったりする。

# NG: コントローラのロジックは再利用しづらい (fatコントローラ問題)
class Products::BulkImportController < ApplicationController
  def create
    Product.transaction do
      params[:product_names].each do |name|
        company.products.create!(name: name)
      end
    end
    redirect_to products_path, notice: "一括登録が完了しました。"
  end
end

# OK: モデルに委譲すればロジックを再利用できる。コントローラも薄く保てる
class Products::BulkImportController < ApplicationController
  def create
    # 一括登録処理はモデル内で実装するように修正
    company.bulk_create_products(params[:product_names])
    redirect_to products_path, notice: "一括登録が完了しました。"
  end
end

カプセル化できているか

オブジェクト指向プログラミングの場合、オブジェクトの外部からオブジェクトの内部状態を参照したり、変更したりするのはNG。そのオブジェクト自身に処理を任せた方が再利用性と可読性が向上する。

# NG: すべてproductに対する問い合わせや操作になっている
if product.registered_on <= 1.year.ago && product.price >= 1_000_000
  product.price -= 10_000
else
  product.price -= 500
end

# OK: 判断や操作はproduct自身に任せる
product.apply_discount

DRY原則を守っているか

「ん？　このロジックはさっきも見たぞ？」と思ったら、DRY原則を適用できる可能性大。

ただし、将来は別々の変更が入る可能性を考慮してあえてDRYにしないケースもあるので、確信がないときはとりあえず「ここはDRYにしないんですか？」と尋ねてみるのがおすすめ。

堅牢性の観点

無関係なデータを1つの配列に格納していないか

無関係なデータを1つの配列に格納して添え字で指定すると、要素の増減に合わせて指定するindexを変えないといけない。ハッシュ（辞書、マップ）なら要素が増減しても指定するキーは変わらないので変更に強い。

<!-- NG: 配列は要素の増減に合わせてindexも変えないと不具合が起きる -->
<% user_data = [170, 65, "AB"] %>
<ul>
  <li>身長 <%= user_data[0] %>cm</li>
  <li>体重 <%= user_data[1] %>kg</li>
  <li>血液型 <%= user_data[2] %>型</li>
</ul>

<!-- OK: ハッシュはキーで指定するので要素が増減しても既存のコードは変更不要 -->
<% user_data = {height: 170, weight: 65, blood: "AB"} %>
<ul>
  <li>身長 <%= user_data[:height] %>cm</li>
  <li>体重 <%= user_data[:weight] %>kg</li>
  <li>血液型 <%= user_data[:blood] %>型</li>
</ul>

なんでもハッシュで管理しようとしていないか

ハッシュは気楽にいろいろなデータを格納できる長所がある一方、キーの指定を間違うと予期せずnil (null)が返ってくる可能性がある。一定の複雑さを超えたら専用のクラスを用意することを検討する。

<!-- NG: うっかりキーをtypoするとnilが返るだけでエラーにならない -->
<li>身長 <%= user_data[:heiht] %>cm</li>

<!-- OK: クラスを定義してメソッドで参照すれば、エラーが発生するのでtypoに気づける -->
<li>身長 <%= user_data.heiht %>cm</li>

文字列結合で○○を生成していないか

○○にはURL、HTML、XML、JSON、SQL、CSVなど、何らかの技術的なテキストフォーマットが入る。

文字列結合は予想外の値がやってくると、フォーマットが破綻する可能性がある。

# NG: user.commentに改行やコンマが含まれているとCSVのフォーマットが破綻する
csv_text = <<TEXT
name,email,comment
#{user.name},#{user.email},#{user.comment}
TEXT
File.open("user.csv", "w") { |f| f.write(csv_text) }

# OK: CSVファイルは専用のライブラリを使って作成する
CSV.open("user.csv", "w") do |csv|
  csv << %w[name email comment]
  csv << [user.name, user.email, user.comment]
end

正しさの観点

ゼロ除算していないか

除算の除数（割り算の割る数）を動的に変更する場合、ゼロ除算になってエラーが発生する恐れがある。そのため、ゼロ除算の可能性を考慮できているか確認する。

# points.sizeが0だとゼロ除算になり、エラーが発生する
def calc_avg(points)
  points.sum / points.size
end

丸め誤差を考慮しているか

整数だけではなく小数も計算対象に含まれる場合、浮動小数点（Float型）特有の丸め誤差が起きないか確認する。特に金額計算では致命的な問題になる可能性がある。

以下は丸め誤差によって割引金額の結果がおかしくなる例（910が正解だが、909になる）。

price = 1300
discount_rate = 0.3 # Float型

# 割引金額の計算（端数は切り捨て）
(price * (1 - discount_rate)).floor
#=> 909

言語によって対応方法は異なるが、RubyであればBigDecimalを使うようにする。

price = 1300
discount_rate = BigDecimal("0.3") # BigDecimalに修正

(price * (1 - discount_rate)).floor
#=> 910

要件を満たしているか

実装がdescriptionに書かれた要件を満たしているかを確認する。また、正常系だけでなく、異常系も考慮できているかどうかも確認する（例: 決済処理が途中でユーザーによってキャンセルされたケースなど）。

セキュリティの観点

フロントエンドだけでなく、バックエンドでも権限チェックしているか

「権限Xを持っているユーザーのみ、操作Yを許可する」という仕様があった場合、「権限Xを持っているユーザーにだけ、操作Y用のボタンを表示する」というように、フロントエンドの制御だけで終わらせるのはNG。

Webアプリケーションであれば、クライアントは自由にリクエストを送ることができるので、バックエンド（サーバーサイド）でも必ず権限をチェックしないといけない。

XSSやSQLインジェクション対策ができているか

XSS（クロスサイトスクリプティング）やSQLインジェクションは、フレームワークやライブラリを素直に使っている限りほとんど発生しないはず。だが、特別な理由があってSQLを文字列結合で動的に構築しようとする場合など、やむを得ずイレギュラーな方法で実装するときは十分注意する。

APIキーなどをコードに直書きしていないか

APIキーなど、クレデンシャルな値をコードに直書きする（＝平文でgit管理する）のはNG。環境変数やフレームワークが用意している仕組み（Railsであればconfig/credentials.yml.encなど）を利用する。

パフォーマンスの観点

本番環境のデータ量を想定できているか

単純なループ処理などは、ローカル環境では一瞬で動作しても、本番環境ではデータ量の違いから異常に時間がかかる場合があるので要注意。集計処理などはプログラミング言語側ではなく、なるべくデータベース側に任せる。

# NG: ユーザー数が何万、何十万もいると単純なループ処理でも時間がかかる
User.find_each do |user|
  user.update!(nickname: user.name)
end

# OK: 単純なデータ更新であればSQL一発で終わらせた方が圧倒的に速い
# (下のコードは UPDATE users SET nickname = name を実行する)
User.update_all("nickname = name")

ロジックの計算量を意識できているか

たとえば、以下のように名称と価格をそれぞれ左寄せ、右寄せで出力したいとする。

Apple        120
Banana         5
Pineapple   3000

以下の2つのロジックはどちらも同じ結果になるが、計算量が異なる。

# NG: ループ内で毎回最大長を求めると、計算量はO(n^2)になり、要素数が増えるにつれて無駄な処理が急激に増える
items.each do |item|
  # 商品名と価格について、それぞれの最大長を求める
  name_width = items.map { |i| i.name.size }.max
  price_width = items.map { |i| i.price.to_s.size }.max

  # 商品名は左寄せ、価格は右寄せでそれぞれ出力する
  printf "%-#{name_width}s %#{price_width}s\n", item.name, item.price
end

# OK: 最大長はループの外で求めておく。こうすれば計算量はO(n)で済む
name_width = items.map { |i| i.name.size }.max
price_width = items.map { |i| i.price.to_s.size }.max
items.each do |item|
  printf "%-#{name_width}s %#{price_width}s\n", item.name, item.price
end

データベースへのアクセス回数を意識できているか

データベース（DB）へのアクセスは基本的に遅い。N+1問題が発生していたり、同じデータを繰り返しDBに問い合わせたりしていると、本番環境のデータ量やアクセス数に耐えきれなくなる恐れがある。

一見、シンプルなループ処理に見えても、その裏側ではDBへの問い合わせが毎回大量に発生している場合があるので注意。

<p>今だけのスペシャルプライス！</p>
<ul>
<% items.each do |item| %>
  <!-- calc_special_price は大量のDBアクセスが発生するメソッドかも？ -->
  <li><%= item.name %>が<%= item.calc_special_price %>円！</li>
<% end %>
</ul>

その他の観点

上で紹介しきれなかった観点を箇条書きで紹介します。コードレビュー時は以下のようなポイントにも注目してみてください。

• その言語・フレームワークの考え方やベストプラクティスに沿った「○○らしいコード」を書いているか（例：Rubyらしいコード、Railsらしいコード）
• そのプロジェクト固有のお作法に従ったコードを書いているか（なお、お作法があるなら暗黙知にせず、明文化しておくことが望ましい）
• 成功・失敗を表すメソッドの戻り値を無視していないか
• 例外処理が適切か（エラーを握りつぶしていないか、キャッチしなくていい例外をキャッチしていないか、条件分岐で実装できるものをわざわざ例外でスロー/キャッチしていないか、etc.）
• DBトランザクションを適切に利用しているか
• アプリ側の変更に対応するテストコードが追加されているか
• テスト設計手法（境界値分析など）を意識したテストコードになっているか
• 複雑な条件分岐を持つロジックに対するテストコードのカバレッジは十分か
• false negativeなテストコード（バグが発生していても検知できないテストコード）を書いていないか

参考：良いコードを学ぶのに役立つ書籍や情報源

前述の「おことわり」に書いたとおり、この記事で紹介したのは「レビューの審美眼を養うとっかかり」になりそうなポイントだけです。「良いコードとは何か」をより深く学ぶために、以下のような書籍や情報源もチェックしてみてください。

言語やフレームワークを問わず役立つもの

qiita.com

RubyやRailsに限定する場合

railsguides.jp techbookfest.org qiita.com qiita.com

コードレビュー時に役立つテクニックや考え方

続けて、「コードレビュー時に役立つテクニックや考え方」を紹介します。毎回漠然とコードレビューするのではなく、対象のPRに合わせて臨機応変にアプローチを変えられるようになりましょう。

自分のことは棚に上げる

「これは自分もできてないんだよな」と思うことでも、コードを良くするためなら躊躇せずにコメントする。また、「コードレビューするときは自分のことは棚に上げてよい」という共通認識をチーム内で持っておく。

権力勾配や面識の有無を意識する

プロジェクト内では同じ職位で、なおかつ互いに面識があるなら、フランクなレビューコメントでも問題ない。

ベテランが新人をレビューする場合など、権力勾配があるときは、レビュアーは物腰柔らかにコメントしないと相手を萎縮させてしまう恐れがある。

また、リモートワークなどで「相手の声も顔も知らない」という関係だと、PR上のテキストコミュニケーションがギスギスしがちになるので要注意。最低でも一度はビデオ会議でお互いの顔を見せながら、あいさつや雑談をする。できればオフラインでリアルに対面するのが望ましい。

良い点があれば「良い」と伝える

コードレビューの特性上、ついダメ出しが多くなってしまいがちだが、良いと思ったところがあれば、「良い」と感想を伝える。

特に、権力勾配があるときは、指摘が入るだけでレビューされる側は「先輩に怒られている」と勘違いしがちなので、少し意図的に褒めた方がよい。

ビデオ会議で画面共有しながらコードレビューする

以下のような場合は、ビデオ会議で画面共有しながら（お互い出社してるなら対面で）コードレビューすることを検討する。

• ロジックがややこしいので、PR作成者に質問しながらレビューしたい場合
• テキストではうまくコメントできない、または言語化にすごく時間がかかりそうな場合
• 内容的にテキストだと角が立ちそうな場合
• 権力勾配があるため、テキストだと「怒られている」と相手が誤解しそうな場合

自動化できる部分は自動化しておく

Linterやコーディング規約チェックツール、セキュリティチェックツールを使った静的コード解析や、AIによるコードレビューなど、自動化できる部分は極力自動化しておく。

とはいえ、ツールの評価結果だけを絶対視しないように。人間によるコードレビューも併用した上で、ツールの設定やルール定義などは、いつでも柔軟に見直せるようにしておく方が好ましい。

好みの問題は「好みの問題」と明記する

「どっちでもいいと言えばいいが、個人的にはこっちが好き」という、好みの議論は「好みの問題」である（直さなくてもよい）ことを明記しておく。

わからないところは「わからない」と質問する

PRのコードを読んでいて、「これはいったい何のコードだろう？」と疑問に思った場合は、素直に質問する。

自信がない部分は他のレビュアーにも見てもらう

自分の技術レベルや、得意分野・不得意分野の関係で、「ここはちゃんとレビューできているか自信がないな」と不安になることもあるはず。そういう場合はその旨を正直に伝えて、他のレビュアーに自信がない部分をレビューしてもらうようにお願いする。

致命的な問題が残っていなければapproveする

指摘内容については、mustなのかshouldなのかを区別しておく。mustな問題が解決していたら、残りの修正はPR作成者にまかせてPR自体はapproveしてもよい。

不具合の発見はあくまでベストエフォート

コードレビューで見つけられる不具合もあるが、コードレビュー自体はテストではないので、レビュアーがすべての不具合を見つける責任を負うわけではない。コードレビューによる不具合発見は、あくまでベストエフォートだと考える。

不具合が発生すると致命的な部分（金額計算や決済機能など）や、複雑なUI制御、複雑な権限管理などは、しかるべき担当者にテストしてもらうように伝える。

あまりにもレビューしづらいPRなら差し戻す

前編では「PRの作成者に配慮してほしいこと」というコラムを書いた。このコラムに書いた事項がほとんど守られず、「こんなPRをレビューするのは、負担が大きすぎる」と感じた場合は、「descriptionを詳しく書いてほしい」「diffが大きすぎるので重点的に見た方がいいファイルをピックアップしてほしい」といったリクエストを付けて、PR作成者に差し戻してもよい。

代替案が出せない場合は、違和感だけ表明しておく

レビューコメントを入れる場合は、なるべく「なぜダメなのか」「どうしたらいいか」をセットで明記すべきだが、ぱっと良い代替案が出せないときもある。その場合は、とりあえず違和感だけを表明してPR作成者にどうすべきかを考えてもらう。

既存コードに問題がある場合も、とりあえずコメントを入れる

コードレビューをしていると、PR作成者がおかしなコードを書いたというより、既存コードの問題が原因でツッコまざるをえないケースもある。そういう場合は「既存コードの問題だと思いますが」と枕詞を付けて、コードの問題点を指摘する。

なお、既存コードに問題があるときは、プロジェクトのコード全体に対して修正を適用しなければならない場合がある（たとえば、サポート電話番号を各画面にベタ書きするのではなく、定数化してそれを参照させたい場合など）。もし修正が広範囲かつ大がかりになりそうなら、別PRでの修正を検討する。

同じコメントが繰り返されるようなら「以下略」で切り上げる

PR作成者の根本的な勘違いなどが原因で、あっちにもこっちにも同じ指摘事項があると、「ここも」「ここも」と繰り返しコメントしないといけなくなる。そういう場合は「あとは自分で見つけて直してね」の意味で、「以下略」と書いて切り上げるのも可。

レビューを受ける側も、指摘コメントを1つもらったら、他に同じような問題点がないか自分で調べて全部直すようにこころがける。

ピンとこなければ実際に動かす

コードレビューはなるべく「コードを見るだけ」で済ませたいが、ものによっては実際に動かしてみないとコメントのしようがないケースもある。そういう場合は、以下のような手段で実際に動かしてみる。

• 自分のローカル環境でアプリケーションを動かす（ただし、自分のローカル環境に必要なデータがなくて動作確認できないこともよくある）
• そのロジックに対応するテストコードがあれば、テストコードをローカル環境で走らせ、デバッガやプリントデバッグで内部的な動きを確認する
• ステージング環境にデプロイ済みなら、そこでアプリケーションを動かす

いずれもダメなら、ビデオ会議などで画面共有しながらPR作成者に実際に動かしてもらう。

頻繁にコメントしている内容は記事にまとめておく

「これ、この間も別のメンバーに同じコメントをしたな」と思ったら、QiitaやZennなどにその内容をまとめておく。そうすれば次回以降は「詳しくはこの記事を読んで」で済ませることができる。

以下は筆者が書いた、そうした記事の一例である。

qiita.com

ただし、業務で書いたコードをそのまま載せることはできないので、サンプルコードは記事用にアレンジしておくこと。

まとめ

というわけで、前編と後編の2回に分けて、コードレビューに関する以下のような内容を説明してきました。

• コードレビューの目的
• コードレビューの進め方
• コードレビュー時に着目すべきポイント
• コードレビュー時に役立つテクニックや考え方

他にも取り上げたいトピックはまだまだあるのですが、これ以上踏み込むと記事のボリュームが大きくなりすぎるため、今回はここまでにしておきます。

コードレビューの経験が浅い若手エンジニアの方は、「こんなにたくさん考えなきゃいけないの？　コードレビューって難しすぎる!!」と思ったかもしれません。たしかに観点やアプローチは山のようにあるのですが、最初から完璧なコードレビューができる人はいません。そこは先輩エンジニアの助けも借りながら、一歩ずつ、レビュアーとしてのスキルを上げていきましょう。

筆者の観測範囲では「コードレビューってどうやるの？」「どういうポイントを見ているの？」といった疑問に体系立てて答えてくれる情報源はそれほど多くありません。この記事がそうした疑問を持つみなさんにとっての「ひとつの手がかり」になれば幸いです。

そして、ぜひこの記事の内容を叩き台として、チーム内で「コードレビューのあるべき姿」について話し合ってみてください。唯一の「正解」はなくとも、自分たちにとっての「より良いコードレビュー」に近づくきっかけにはなるはずです！