弊社ではGithubのプルリクエスト機能でコードレビューを行っているのですが、どのような目的・観点で行うかという基準はレビュアーに委ねられている状態です。
この状態ですと以下のようなリスクがあるので、会社として「このような基準でレビューを行う」というルールを整備していきたいと思っています。
- レビュアーによってフィードバックの内容が異なり、一貫性がなくなる
- 新たにレビュアーになった人が何を基準にしたらいいのか分からなくなる
- 開発者も何に気をつけてコーディングすればいいのか分からなくなる
コードレビューの基準を考えるにあたって調査を進める中で、Googleのエンジニア向けのコードレビューのドキュメントを見つけました。
Google’s Engineering Practices documentation 日本語訳
https://fujiharuka.github.io/google-eng-practices-ja/ja/review/reviewer/standard.html
今回は上記のドキュメントを紐解きつつ、コードレビューがいかに行われるべきかを考察していきたいと思います。
コードレビューの目的
まずはコードレビューの目的ですが、ドキュメントには以下のように書かれています。
コードレビューの主な目的は、Google のコードベースにあるコードの全体的な健康状態を時間をかけて改善することです。
（省略）
レビュアーはコードベースの一貫性と保守可能性を維持し、「コードレビューの観点」で言及されている事項を守りたいと考えています。
特に「保守可能性」は実務上とても重要です。
というのも、一度の開発で完結するプロジェクトはごく僅かで、大抵は半年から1年ほど期間を空けて、運用を通して見えた課題を第２・第３フェーズで解消していくからです。
期間が空くと開発者自身も記憶が曖昧になりますし、以前アサインしていなかったメンバーが入ったりするので、どうしてもソースコードを読解するという工程が発生します。
このときにソースコードをパッと見で理解できなかったり、複雑すぎて下手に直すとバグに繋がる恐れがあると、余計なストレスがかかりモチベーションが低下にも繋がります。
ソースレビューでどこまで指摘すべきか？
どこまでフィードバックして修正してもらうべきかは、開発期間やリソースとの兼ね合いもあるので悩ましいポイントです。
この点に関して、ドキュメントには以下のように書かれています。
「完璧な」コードといったものは存在しないということです—存在するのはより良いコードだけです。 レビュアーは CL の承認を保留したままコードのすみずみまで磨きをかけることを要求すべきではありません。
（省略）
レビュアーは何か改善できそうな点を見つけたら、コメントを残すのに躊躇する必要はありません。いつでも気軽にコメントを残すべきです。 その際に、もし重要でない指摘であれば、「Nit: 」（あら探しや細かい指摘という意味）のようなプレフィックスを付けて、 これはただ完全を期すための指摘なので無視してもらっても構わない、と CL 作成者に知らせると良いでしょう。
自分なりに解釈しますと、フィードバックを以下の２つに分けるということだと思います。
- 必ず直して欲しいところ（仕様との齟齬、バグに繋がりそうな箇所、コーディングルール違反など）
- 直すと尚良くなるところ（経験則上このように書いた方が良いといったアドバイス）
特に後者については、隅々まで修正して膨大な時間を割くのは開発スピードやコストの面であまり良くない気がしますし、かといって省くと開発者の気付きや成長に繋がりません。
ですので、あくまでアドバイスとしてコメントを残しておき、実際に修正してもらうかどうかは状況次第にするというスタンスは非常に実践的です。
コードレビューの観点
コードレビューの観点については、別ページにまとめられていますが、理解を深めるために、５つの観点それぞれを実際のプロジェクトに当てはめて考えていきたいと思います。
コードの設計
弊社のプロジェクトの多くは、フロントエンドにVue.js（＋vuex）やReact（＋redux）、バックエンドにLaravel（＋リポジトリパターン）を採用しています。
例えば、LaravelでAPIを作成するときに複数のクラスを使い分けますが、「どこに何を書くべきか？」はアーキテクチャの意図によります。
各クラスの目的を以下のように定義したとして、全てのコードが目的どおりに書かれていれば、欲しい情報がどこに記載されているか分かりやすくなり、デバッグや機能追加も容易に行えるようになります。
Model
DBと情報をやり取りするクラス
Repository
データを永続化（DBに保存）したり、永続化したデータを取得したりするクラス
Service（UseCase）
データの永続化を含まないビジネスロジックを処理するクラス
例１）データ登録時にユーザーに通知する
例２）あるデータを削除した際に関連データも併せて削除する
Controller
ViewとService（またはRepository）の間で、データの受け渡しを仲介するクラス
コードレビューでは、開発者が書いたコードがアーキテクチャの目的に沿って書かれているか確認することで、全体の一貫性や保守性を維持しやすくなります。
注意点としては、アーキテクチャはアサインするプロジェクトによってかなり異なるので、特に開発者の方はあらかじめ開発リーダーなどにアーキテクチャの全体像や意図などを確認しておくことをおすすめします。
機能性
こちらは開発者側で充分な単体テストが行われていることが前提として、以下のようなポイントを確認していきます。
エッジケースを想定できているか？
エッジケースとは、発生する可能性は低いがまれに起こりうるイレギュラーなバグや脆弱性のことを指しています。
たとえ発生確率が１％であっても、利用者数が月間１０，０００人を超えるサービスであれば１００人程度が遭遇する可能性あるので注意が必要です。
【例1】外部的な要因による機能不全
インターネット接続の不具合が起きている場合などに、適切な注意を促しているか？
【例2】不正アクセス
ユーザーが権限上アクセスできないページを訪れたときに、適切なエラーページ（403 Forbiddenなど）を返せているか？
【例3】ファイルの拡張子偽装
ファイルのアップロード機能で拡張子を制限している場合に、ユーザーが拡張子を偽装してアップロードするケースに対応できているか？
快適に利用できるか？
ユーザーの立場になって動作確認を行い、システムのパフォーマンスをチェックしていきます。
- フォーム画面の登録／更新にかかる時間は短いか？
- 一覧画面の検索にかかる時間は短いか？
- 画面の遷移や挙動におかしな点はないか？
複雑性
複雑性とは何かという点に関しては、以下のように書かれています。
「複雑すぎる」とは普通、「コードを読んですぐに理解できない」という意味です。
あるいは、「開発者がこのコードを呼び出したり修正したりしようとするときに不具合を生み出す可能性がある」という意味でもあります。
ポイントして上げるとすれば以下のような点になると思われます。
長くないか？ → 分割する
不要なものがないか？ → 必要なものだけに絞る
分散（重複）していないか？ → 共通化する
余計なこと（オーバーエンジニアリング）をしていないか？
気を利かせて書いたつもりでも実は全体の一貫性を損なってしまう場合があります。
例えば、上のアーキテクチャの例でいいますと、もともとModelとControllerのクラスだけで構成していたにも関わらず、新たにRepositoryを覚えたので勝手に導入してしまうケースです。
エンジニアたるもの新しく覚えたことをすぐ実践したくなる気持ちはわかりますが、「郷に入れば郷に従え」の精神で自分に与えられた課題に集中することが大切です。
命名
ファイル名・クラス名・関数名・変数名がわかりやすく付けられているかチェックします。
let count = 0 // 何のカウントか不明
↓
let validDataCount = 0 // 有効なデータ数だとわかる// 何のデータを取得しているのか不明
function getData() {
~~~~~~~~~
}
↓
// 請求書のデータを取得しているのだとわかる
function getInvoice() {
~~~~~~~~~
}短すぎず、長すぎず、汎用的過ぎず、略し過ぎずというバランスを取らないといけないので、私自身考えていて難しいことも多いです。 困ったときは「codic」などのサービスを使って、良さそうな命名を探すといいかもしれません。
コメント
今後ソースコードをメンテナンスする人の立場になって、意味のあるコメント／意味のないコメントを精査していきます。
意味のあるコメント
パッと見でわからないこと
switch ($fruit_type) {
case 1: // 柑橘類
case 2: // 仁科類
case 3: // 核果類
default:
}今後、行う必要があること
// ToDo: ~~~~~~~意味のないコメント
コードを見たらすぐにわかること
// リクエストにtypeが存在しNULLでない場合
if ($request->filled('type')) {
~~~~~~~
}とはいってもなかなか区別が難しいところがあると思うので、パッと見で分かりづらいところにコメントがしっかり書かれているか確認する方が現実的かもしれません。
コードレビューのコツ
一行ずつ見る
忙しいからといってコードに書かれていることが正しいとは決めてかからず、一行ずつじっくり読むことが大切です。
また、コードをレビューしている段階でどこを直したのか分からない／読みづらいといったレビューを妨げるようなことがある場合は、一度開発者に戻して書き直してもらうように依頼するのも良いと書かれています。
レビュアーがパッと見で分からない時点で、他のメンバーが見ても分からないことは確実ですし、難解なコードを一つひとつ読み解くのは忙しいレビュアーにとっては時間の浪費になるからです。
良いことは褒める
マネージメントする立場になるとつい忘れがちですが、自分が創意工夫したところに気付いて褒めてもらえるのは嬉しいですし、やる気も出ます。
私自身、レビューをしていてかなり忘れがちな点なので、今後は意識していきたいと思いました。
コードレビューは間違いにばかり目が行きがちですが、良い実践に対しての励ましや感謝の言葉も伝えるべきです。
メンタリングの観点では、開発者が正しいこと行ったときにそれを伝えるほうが、間違いを指摘するよりもずっと価値がある場合があります。
