チームでソフトウェアを開発するとき、ソースコードレビューをすることがあると思います。私もレビューをよくするのですが、やる度にレビューの観点が変わってしまっているような気がしていて、一度整理しようと思いました。
この記事では、ソースコードレビューとはなにか、目的ややり方、観点のチェックリストについて書いています。レビューをするときの参考になればと思います。
この記事の対象は、ソフトウェア開発に携わるエンジニアの方です。私はウェブアプリケーションのフロントエンドやバックエンドに関わることが多いので、それ以外の領域については視点が欠けているかもしれません。
テクニカルライター。元エンジニア。共著で「現場で使えるRuby on Rails 5」を書きました。プログラミング教室を作るのが目標です。
ソースコードレビューとは
ソースコードレビューとは、ソースコードの差分を見て、コメントなどの手段をとおしてソースコードの品質向上を促すことです。役割としてはふたつあって、レビューを依頼するレビューイと、レビューするレビュアーがいます。レビュアーは複数人の場合もあります。
ソフトウェアの品質を担保するために、レビュアーの許可がないと変更を適用できない、といったルールで運用されるところもあると思います。GitHubのプルリクエストや同様の機能をとおして行なったり、対面やオンラインで集まって行うこともあります。
この記事では、「プルリクエストをどう書くか」といった内容は書きません。これについては「プルリクエストの書き方」で解説していますので、あわせてご覧ください。
ソースコードレビューの目的
ソースコードレビューには、大きく分けて次の4つの目的があります。
- ソフトウェアの品質を高めるため
- バグなどの問題の混入を防ぐため
- 影響範囲を確認するため
- メンバーの教育のため
1と2はレビューの目的として、なんとなく想像がつくと思います。3の影響範囲については、その変更がソフトウェアの別のところ、ソフトウェア以外にもユーザーやオペレーションなどに影響を及ぼさないか？などを確認します。とても重要な目的といえます。
また、ソースコードレビューは教育の効果も大きいです。レビューイはもちろん、レビュアーにとっても学ぶことがあったりします。
ソースコードレビューのやり方
まず、ソースコードレビューのやり方について、次の7つの項目でまとめます。
- コメントの種類を押さえる
- コメントの仕方に気をつける
- よいところもコメントする
- 根拠を示す
- 進捗に応じて見るポイントを変える
- レビュー後でも直せるかどうかを意識する
- 口頭でのコミュニケーションも併用する
1. コメントの種類を押さえる
ソースコードレビューのコメントは、レビューイがなにをすべきかが自明であるように書きます。レビュアーの考えを深読みしなくていいようにします。このやり方として、次の3つを使う方法があります。
| 項目 | 意味 |
|---|---|
| MUST | 対応すべきこと |
| IMO | 自分の意見（In my opinion） |
| nits | 細かい指摘 |
他にもSHOULDを使う例をよく見かけますが、対応すべきかどうかが曖昧なのでMUSTかIMOに寄せるべきだと私は考えています。
2. コメントの仕方に気をつける
ソースコードレビューのコメントはコミュニケーションでもあるので、コミュニケーションが円滑になるよう絵文字などを交えて投稿した方がいいと思います。
3. よいところもコメントする
2と同じ理由ですが、ソースコードレビューはコミュニケーションであるので、相手をほめることも意識した方がいいと思います。また、よいところをコメントすることはメンバーの学びにもつながります。
4. 根拠を示す
コメントが「こうすべき」だけだと、レビュイーは「なぜ？」と思ってしまいます。その根拠として、参考文献などをリンクで示すと親切です。
5. 進捗に応じて見るポイントを変える
作業中のソースコードをWIPとして先にプルリクエストを出すやり方もあります。WIPのときは変更の前提となるところ、例えば「そもそもこの変更は必要なのか」や「設計や実装の大まかな方針に問題はないか」を見ます。
このとき、細かいスタイルについては指摘する必要はありません。実装完了後に、具体的な実装方法や書き方についてレビューしていきます。
6. マージ後でも直せるかどうかを意識する
たとえばビューの修正はマージ後でも低コストでできますが、データベースの変更は修正コストが大きくなります。データベースのように、変更しづらいところを特に意識してレビューした方がよいといえます。
7. 口頭でのコミュニケーションも併用する
コメント上だけでのコミュニケーションが難しい場合は、口頭でのコミュニケーションも併用した方がいいことがあります。ただこの場合でも、指摘内容はコメントしておいた方が、どういう議論をしたかが分かりますし、他のメンバーの学びにもつながります。
ソースコードレビューの観点のチェックリスト
それでは次にソースコードレビューをどのような観点で行うか、チェックリストを示していきます。これはチームによって変わってくると思うので、参考程度にしつつ、チームごとのチェックリストをつくれればと思います。
前提
- ソースコードを変更する目的に問題はないか
- ソースコードの変更項目に問題はないか
- コミットの粒度は問題ないか
- コミットメッセージは問題ないか
ソフトウェアの動作
- 画面の表示に問題はないか
- 文言に問題はないか
- 満たすべき環境すべてで問題なく動作するか
設計方法
- 変更項目と実装内容に齟齬はないか
- ロジックは合っているか
- よりよい設計はないか
- オブジェクト指向設計の原則は守られているか
実装方法
- 不要になったソースコードはないか
- DRYに書けているか
- 将来必要になるかもしれないものを実装していないか
- 導入したライブラリに問題はないか
- 使えそうなライブラリはないか
- 標準のAPIにある機能を再発明していないか
- 使用できる既存のモジュールはないか
- APIのリクエストやレスポンスに問題はないか
- リトライ処理は行えているか。問題ないか
- データの型は適切か
- データの不整合は起こらないか。トランザクションは張れているか
- ログは適切に記録されているか
- デバッグ用のコードは残っていないか
- よりよい書き方はないか
- 命名に問題ないか
- 名前から副作用の有無は想定できるか
- 名前と実装は合っているか
- 可読性に問題はないか
- ガード節を用いているか
- アクセス修飾子は適切か
- スコープは適切か
- スタイルに問題はないか
- ひとつのメソッドの処理は多すぎないか
- ネストは深くないか
運用
- ユーザーに影響はないか、対応はとれているか
- メンバーに影響はないか、対応はとれているか
- システムに影響はないか、対応はとれているか
- ドキュメントの更新は十分か
- 法的、あるいは利用規約上の問題はないか
テスト
- テストケースに問題はないか
- テストの実装方法に問題はないか
- テストはパスしているか
- セキュリティ上の問題はないか
パフォーマンス
- パフォーマンス上の問題はないか
- N+1問題は発生しないか
- 計算量は問題ないか、あるいは減らせないか
- クエリは最適化できないか
よりよいソースコードレビューのために
ソースコードレビューをよりよくするためには、まずレビューの観点をチームで共有しておくべきだと思います。レビューで、私もたまにあるのですが、コメントを受けてモヤモヤすることがあると思います。それはチームとしてのレビューの観点が決まっていないから、というのが理由のひとつにあると思っています。
レビューの観点を共有しておけば、レビュアーはその観点に基づいたコメントであるという担保ができますし、レビューイもコメントではなく観点自体についての建設的な議論を提案できます。チームで実践しているふりかえりなどのプラクティスで話せるとよさそうです。
また、自動化できるところは自動化すべきといえます。たとえば細かいスタイルについての指摘は、する側も受ける側もあまりいい気持ちになれないと思います。こういったところはフォーマッタやソースコードレビューサービスなどを利用して自動化するといいと思います。
まとめ
ソースコードレビューは、ソフトウェアの品質向上のための重要な取り組みのひとつです。やり方や観点を抑えておくことで、効果的なレビューを行えるようになります。この記事がレビューをするときの参考になればうれしいです。
