コードレビューのとき、私は何をレビューしているのか？

この記事は、Money Forward Engineering 1 Advent Calendar 2022 10日目の投稿です。
9日目はSaikiさんで 経験0からスクラムマスターを始めるためにやったこと | Money Forward Money Forward Engineers' Blog でした。
本日はcabossoldirが表題のことについて書いていきたいと思います。

この記事の背景

私はいくつかのメガベンチャーと呼ばれる規模の会社で、Web系のソフトウェアエンジニアとして数年間仕事をしてきました。そして、新卒1年目の仕事に慣れ始めたタイミングから、チームメンバーの様々なコードレビューをするようになりました。しかし、当初から何らかの観点を持っていたわけではありませんでした。

そういった形で始まったコードレビューという業務ですが、何年か続けていく中で、何らかのフロントエンド/サーバサイド/IaCのどれにでも適用可能な抽象的なフレームワークのようなものが自分の中に見えてきたので、それを言語化しようとこの記事を書き始めました。

この記事の目的

この記事を、私が書き、誰かが読むことによって、私にとっても誰かにとっても何かが得られるものになっていればと考えています。

私にとってこの記事を書く目的は、現時点で私が考えているコードレビューを言語化し、これからのコードレビューという業務でやることを形式化し効率を上げることです。
そして、私の周りで働くエンジニアがこの記事を読むことで、私のコードレビューで指摘されることを前もって満たし、より効率的に開発を進められればと考えています。
また、私の周りで働いていないエンジニアであっても、いきあたりばったりのレビューしかできない/されないことに対して何らかのヒントになれば良いかなと考えています。

前提とする知識

本題に入る前に、「システム開発の工程」について触れておこうと思います。
ウォーターフォール開発の中でよく見られる「要件定義」「仕様策定」「設計」「実装」「テスト」などのそれぞれの工程についてです。

アジャイル、スクラム、カンバンなどのやり方で仕事を進める中でも、これらの工程に整理することは可能だと考えています。
また、整理すべきだとも考えていて、この整理をもとにコードレビューをしています。

※私は要求定義まで上流の工程に十分に関わって仕事ができているとは言い切れなく、またコードレビューとは少し離れてしまうため、この記事で扱うのは要件定義までとします。
※テストには、結合テスト、システムテスト、受け入れテストなどがありますが、コードレビューに関することに焦点を当てたいので、この記事で扱うのはテストコードで担保できるものまでとします。

要件定義

システムが何を満たせばよいのかをまとめる工程です。
顧客や市場の要求をエンジニアがすり合わせ、分析することで、まとめることができます。

この工程を経ることによって、

• システムにどのような機能があればよいかという項目がまとまった機能要件
• 機能以外にシステムが満たすべき項目がまとまった非機能要件

が、UML等を混じえて説明されるドキュメントがアウトプットとして出てきます。

仕様策定

システムが要件を満たすためにどのような挙動をすべきかをまとめる工程です。
要件をもとに、発生しうる状態のパターンを洗い出したり、UIを考えたり、APIの input/output を考えたり、etc...etc...ということを行う工程です。

この工程を経ることによって、例えば下記のようなアウトプットができます。

• 画面の挙動が確認できるワイヤーフレーム
• API の挙動が確認でできるドキュメント（e.g. OpenAPIなどの定義書）
• データの状態に合わせて発生しうる状態のパターンが書き出されているリスト
• 正常系/異常系の挙動が書き出されているリスト
• etc...etc...

設計

仕様を満たすために、どのような方法で実現するのかをまとめる工程です。
システムの挙動が決まった部分から、この工程に進むことができます。

この工程を経ることによって得られるアウトプットは多岐にわたるのですが、例えば以下のようなものがあります。

• システム全体の概要を示す構成図
• テーブル構造が記述されている定義書（= ER図）
• クラス同士の関係が記述されている定義書（= クラス図）
• 実装内容が細かく記述された詳細設計書
• etc...etc...

実装

設計に沿って、実際にコードを書く工程です。
この工程で、アウトプットとしてコードや、「何らかの形で動く」システムが得られます。

テスト

実装したコードが正しく動くか検証する工程です。
コードレビュー時点では、下記のようなアウトプットが出せるのではないかと考えています。

• 自動で正しさを担保するための、テストコード/実行履歴
• 手動で正しさを担保するための、テスト手順書/実施履歴

何をレビューしているのか？

要件をレビューしているのか？

要件は可能な限り見ていますが、今後のレビューのためのキャッチアップの意味合いで目を通すという方が近いかもしれません。・

開発が始まる前に、プロダクトマネージャ等の役割を担う人がユーザや市場から要求を何らかの形でまとめているかと思います。
まとまっている要求を元に、まずは下記のようにキャッチアップを行っています。

• 実装までの工程に関して確からしさをレビューするため、何が要件かを確認する
• 今後機能が拡張されるイメージを持つため、何が要件にならなかったのかを確認する

キャッチアップした要件を元に、下記をレビューしています。

• 要求を元に、最低限満たすべき機能要件が定義されているか？
• 機能以外に何を担保しなければならない非機能要件を自ら定義しているか？

ただし、この要件定義は抽象度が高く、コードレビューでレビューしきることには限界はあるため、可能な限りとしています。

また、コードレビュー時に要件に関して指摘があると大きく手戻りになるります。そのため、機能の規模が大きい場合は、なるべくコーディングの前段階のアウトプットに対してレビューを行いたいと考えています。

仕様をレビューしているのか？

仕様も可能な限りレビューをしています。特に下記のような観点でレビューをしています。

• 要件を満たせるシステムの挙動になっているか？
• 今回は満たさないとした要件が、
  • 今後満たされるされる可能性がある場合、それを阻害するような仕様になっていないか？
  • 今後満たされる可能性がない場合、可能な限り可能性がないことを伝えられる仕様になっているか？
• 発生しうるパターンが例外を含めて網羅できているか？

特にわかりやすく見れるのは、「発生しうるパターンが例外を含めて網羅できているか？」で、

• APIのレスポンスが2xx系になるパターンだけでなく、4xx系になるパターンが全て洗い出されているかー
• 既存のデータパターン全てに対応した挙動が全て洗い出されているか？
• etc...etc...

という形で、発生しうる例外を自分でも考えてみてレビューをするとともに挙動をキャッチアップしています。
仕様のキャッチアップは後述する、「テストを見ているのか？」でレビューするときに重要な情報になってきます。

また、仕様についても要件ほどではなないですが、コードレビュー時に指摘があると大きく手戻りになります。そのため、機能の規模が大きい場合は、なるべくコーディングの前段階のアウトプットに対してレビューを行いたいと考えています。

設計をレビューしているのか？

設計も可能な限りレビューをしています。特に下記のような観点でレビューをしています。ほぼ仕様と同等です。

• 仕様を満たす実現方法になっているか？
• 今回は満たさないとする要件の仕様が、
  • 今後満たされる可能性がある場合、それを阻害するような設計になっていないか？
  • 今後満たされる可能性がない場合、可能な限り可能性がないことを伝えられる設計になっているか？

設計については、例えばサーバサイド/クライアントサイドのみの小規模なロジックを修正する場合などは、詳細設計を省いている状態でもしっかりとしたレビューは可能で、それほど手戻りも少ないレベルだと考えています。
しかし、DBの変更を伴ったり、クラウドサービスの構築を含む場合など、アプリケーションのロジック以外を含めた範囲に及ぶ修正が必要な場合の設計は、コードレビュー時に指摘があると大きくて戻りになります。このような場合も、コーディングの前段階のアウトプットに対してレビューを行いたいと考えています。

実装をレビューしているのか？

こちらは、本題ですのでレビューをしています。
これまで行ってきた設計までのキャッチアップをふまえて、

• 仕様を満たす実装がされているか？
• 満たされたなかった要件、仕様、設計について、今後満たされるべき/満たされないべきかを伝えられる実装になっているか？

を特にレビューしています。

テストをレビューしているのか？

ここでは、テストコードと手動で実施しなければならない手順書を確認しています。
どちらも、キャッチアップした、もしくはレビューして修正された仕様を元に、「それらのパターンを網羅したテストができているか？」をレビューしています。

特に、ここでキャッチアップした仕様を網羅したテストコードがあるかどうか？をレビューしきれるかどうかが、意図しない挙動を見つけられるかどうかに関わってくると感じています。
そのため、レビューの過程の中では、「仕様のキャッチアップでパターンを網羅すること」と「パターンを網羅したテストコードが書かれること/テストが実施されること」を最も気を使ってレビューしています。

コードより先に見れると楽なもの

要件/仕様/設計は、実装よりも上位の工程にあたるため、コード以外のアウトプットでレビューすほうが、開発全体が効率よく進むと考えています。

要件定義時点

要件定義の時点で下記のようなアウトプットがあれば、コードレビュー前に確認できると良いかなと思っています。

• 機能追加をするための企画書（スライドやDocsなんかにまとまっている、誰かが機能について説明に利用したアウトプット）
• 機能要件一覧がまとまっているリスト・UML等で図解したドキュメント
• 非機能要件がまとまっているリスト

仕様策定時点

仕様策定の時点で下記のようなアウトプットがあれば、コードレビュー前に確認できると良いかなと思っています。

• 仕様一覧がまとまっているリスト・UML等で図解したドキュメント
  • e.g. APIドキュメント
  • e.g. 画面の挙動がわかるもの（ワイヤフレーム、デザイン）

特に、デザインに関してもエンジニアのレビュー対象だと考えています。ソフトウェアエンジニアとしては、要件を満たしているソフトウェアをリリースする責任があるためです。

設計時点

設計の時点で下記のようなアウトプットがあれば、コードレビュー前に確認できると良いかなと思っています。

• 設計がまとまっているリスト・UML等で図解したドキュメント
  • e.g. テーブル設計がまとまったドキュメント
  • e.g. クラス設計がまとまったドキュメント
  • e.g. クラウドサービスの構成図

まとめ

コードレビュー時点でもキャッチアップを目的とし、要件/仕様/設計をなるべくできる範囲でレビューをしています。
ただし、コードレビュー時点ではかなり巻き戻る類の指摘をすることもありうるため、規模によってはコード前のアウトプットでレビューできれば嬉しいです。

特に、仕様のキャッチアップとテストで担保できてるかに一番気を使ってレビューをしています。