コードレビューで個人的に意識していること

こんにちは。
PharmaX でエンジニアをしている諸岡（@hakoten）です。

この記事の概要

この記事では、私が普段コードレビューで意識していることを紹介しています。
コードレビューのルールというより、個人としてのルーティンや意識していることについて書いています。

※ チーム自体の「レビュールール」は別で存在していますので、ルールに沿った上でのルーティーンや心構えという認識で読んでいただければと思います！

レビュールールの運用やルール自体については触れていませんので、ご了承ください。

自身のキャリア

どのような経歴のエンジニアからの話なのかを知っていただくと、内容を想像しやすいかと思いますので、私のエンジニアとしての経歴を簡単に紹介します。

• ソフトウェアエンジニア 14年
• 主な領域、モバイル、ウェブ、APIサーバーなどのアプリケーションメインのエンジニア
• 機能リード、テックリードあたりをやることが多い
• 主に小規模チームでプロダクト開発やプロダクト運用をしていることが多い

前提となるレビュー環境

レビューを行う際の前提条件として、以下の環境設定が整っていることを想定しています。レビュー時にはこれらの点は特に考慮しておらず、レビュー前に以下の条件を満たすように環境を整備しておくことが望ましいと考えています。

• 適切なリンター、フォーマッターが組み込まれており、静的チェックは自動化されている
• 単体テストが書かれており、テスト実行は自動化されている

基本的に人の手を借りずにチェックできることは、あまり重要視せずに自動化でカバーする方向性です。

基本のレビュールーティーン(機能追加や機能改修時)

普段レビューをする時にどんなフローでコードを読んでいるかを記載します。
ここでは主に「機能追加や機能改修」について書いていますので、次のようなケースは対象外となります。

• 同じ修正を多数のファイルに適応している:
  • 明らかに同じ修正であれば、修正内容を理解し、軽く全体を確認するようなことが多いです。
• バグ修正:
  • その修正が妥当であるかを、修正コード以外の箇所も含めみています。混乱するので今回はスコープから外しています。
• その他:
  • その他、例外ケースもあるため、必ずしもこのフローを徹底していないこともあります。

① PullRequestの説明文を読んで修正内容を理解する

当然ではありますが、まずはPullRequestの説明文を読みます。
この時、修正内容に不明点があれば、本人に内容を確認することもあります。

また、仕様ドキュメント、デザインドキュメントなどがあれば、そちらにも目を通し「どんな要件を実装しているのか」「どの辺りのコードを修正しているのか」をコードを読む前に簡単に理解するところから始めます。

② コードの修正範囲を全体的に俯瞰する

PullRequestの内容を読んだら、コードのFiles changedを開いて見ていきます。
まず行うのは「コードの修正範囲を全体的に俯瞰する」ことです。

変更されたファイルの数にもよりますが、「じっくり読むべきファイルはどれか」「ざっと目を通すだけで良いファイルはどれか」を大まかに分類し、場合によってはGitHubのViewedチェックを付けて閉じます。

自動生成されたファイルや、画像ファイルなどみなくて良いファイルはViewedにチェックしていく

例えば、自動生成ファイルや画像ファイルなどは、閉じてしまうことが多いです。

③ 起点のコードから処理を追っていく

コード全体の差分の様子と、どんな修正をしているかの概要を理解したら、改修した処理の起点からコードを読んでいきます。APIであれば、ルーティング → コントローラーへ、Reactのコードであれば、修正された画面のコンポーネント → 新規追加のコンポーネントへ、修正の全体処理の流れを追ってコードを読みます。

④ 質問やコメントを書いていく

読んでいく過程で、出た疑問やコメントはあまり精査せずどんどん書いていきます。(観点は後述)
最終的にレビューをFIXするまでは通知されないため、書いてある内容の妥当性は気にせずに、感じたまま書いていきます。

⑤ モジュール・ファイル・関数・コンポーネント単位で細かく読む

全体の処理の流れでコードを読んだら、モジュール単位や関数、コンポーネント単位でもう一度読みます。
ポイントは処理の流れのことは一旦頭から忘れて、その関数やコンポーネント単体として責務がどうなっているか?という観点で見ることです。

⑥ 質問やコメントを書いていく2

基本④と同じで、気にせずどんどん書きます。

⑦ 処理の先頭からコメントと一緒にもう一度読む

ここまでで書いたコメントを自分で確認しながら、コードをもう一度処理の頭から読みます。
この時、理解度が上がったことで不要になったコメントは消していきます。また内容が変わったコメントも修正していきます。
もうレビューが済んだファイルについては、githubのViewedチェックを付けて閉じていきます。

⑧ 完了

全てのコードに、Viewedチェックを付け終えたら基本的に完了です。
重要な機能であったり複雑な機能の場合は、ここまでの流れを何度か繰り返すこともあります。

レビューの観点

ここでは、個人的に重要視しているレビューの観点について書きます。
冒頭にも書いていますが、チームルールとして観点も存在しているため、そちらも包括した上で個人的に重要視しているものになります。

重要視している: 保守しやすいコードになっているか

運用フェーズにおいて個人的に重視しているのは、そのコードが「保守しやすい作りなのか」という観点です。具体的には次のような箇所を見ています。

周辺コードと設計や方針(ルール)がずれていないか？

例えば、類似する機能や影響する機能「API①」「API②」「API③」があり、修正が入っている処理が「API①」だとします。

この場合は、「API②」「API③」についても関連コードとして一緒に読み、修正があった「API①」の設計方針や細かい書き方などが他コードと合っているかを確認しています。

このケースは、レビューイよりもコード理解度が高いという前提にはなりますが、類似・関連処理に気づかずに全体の設計が歪み、後のメンテナンス性が下がらないように意識しています。

難解な実装になっていないか？

もともとの要件が複雑であったり、難易度が高い実装の場合は仕方がないこともありますが、過度に難解であったり、高度な実装になっていないかも意識しています。

一見するとエレガントに見える実装でも、チーム間でメンテナンスするには難しすぎるように感じた場合は、そのまま正直に内容を書きます。

この辺りはとても難しい問題ではありますが、「ドメイン知識、コード知識が高く、経験も豊富なベテランしかいないチーム」なのか「比較的若手が多いチーム」なのか、状況によっても変わるかもしれません。(ベテランしかいないのであれば、問題ないためコメントしないこともあります)

モジュール・クラス・コンポーネントのインタフェースが適切か?

モジュール、クラス、コンポーネントの単位で、適切なインタフェースが切られているかを気にしています。

インタフェースが適切であれば、大きく間違いが発生しないのがプログラミングの特性ではあるため、インタフェース(メソッドの責務、引数や戻り値の妥当性など)やファイル分割、モジュール分割が適切なのかは注意しています。

変数名、関数(シグネチャ)、コメントが適切か？

コードにおいて、名前の付け方は大切ですが、自分の書いたコードには慣れてしまっているため違和感を感じにくいものです。また、他者の書いたコードであっても、１度理解したり、コミットされてしまうと違和感に気づきにくくなってしまいます。

そのため、「レビューで初めて定義される名前は、最も新鮮な状態で違和感に気づけるチャンス」と考え意識して読んでいます。

コードをフラットな目線で読み「名前として変数や関数の意味がわかるか？」「コメントの内容が理解できるか？」を何度か繰り返し、小さな違和感でもそのままコメントするように心がけています。

適切な粒度でテストケースが存在しているか？

後述していますが、「正しく動作するか」の確認はあまりしないため、「正しく動作することが担保できるテストケースが書かれているか」の確認をしています。
テストの細かい書き方などはあまりみていませんが、最低限の仕様担保ができているかをみています。

あまり重要視していない: 正しく動作しているかどうか

チームの方針も影響しますが、基本的に動作確認は実装者本人が行うため「正しく動作しているか？」を動かして確認することはありません。(自分自身が、詳細を理解するために動作確認でコードを動かすことはあります)要件を満たせているかも基本的には実装者が確認している前提で読んでいます。

とはいえ、コード上で明らかに間違っている箇所や、要件不備を見つけたらコメントはします。

レビューイの背景によってレビュー観点はカスタマイズする

レビュー観点ついては、レビューイがどんな人物なのかによって多少カスタマイズしています。

例: コード理解が高くないメンバー

チームに入って間もないコード理解が浅いメンバーは、既存実装について完全には把握できていません。そのため次のような観点を取り入れています。

重複実装が発生していないか

「存在するコードで流用できるものを新規で書いていないか？」「定数など重複しているものを追加していないか？」をみています。

これは相手に早くコード理解度を高めてもらう意味でも重要だと考えているため、使用箇所や使い方も一緒に書くことを心がけています。

影響範囲の考慮が漏れていないか

長く運用されているコードには、初見ではわからないような難解な影響範囲が潜んでいるものです。新しく入ったメンバーの場合は、複雑な影響範囲には気が付きようがないため、この辺りも慣れるまでは注意深く見ています。

例: 経験が浅い新卒やインターン

経験が浅い新卒やインターンの場合は、本人の学習観点も考慮しています。普段はあまり意識しないことも追加で考慮しています。

実装要件が満たせているか？

仕事自体の経験が浅いということもあり、「きちんとチケットの意味を把握していそうか？」という点も慣れるまでは見ています。実装要件のポイントを把握できていないこともあるので、要件がきちんと満たせているかも確認することがあります。

自分で書いたコードの意図を理解しているか？

業務コーディングに慣れるまでは、「なんとなく書いているコード」が意外にあったりします。あまり理解してなそうな雰囲気を感じるコードを見かけたら、意図を理解しているか質問など書き確認しています。

もっと良い(効率的な)書き方がないか？

普段であれば、あまりにパフォーマンス効率が悪くなければ、実装方法はあまり気にしなかったりしますが、経験が浅いレビューイの場合は、「こんな書き方もある」という意味も込めてコメントしています。

自分自身のこころ構え

チームルール上の心構えもありますが、最後に個人として大事にしている心構えを書いておきます。

理解できなくても、全ての行を読む

状況によっては、自分の守備範囲外のコードをレビューすることもあります。そういう時でもおざなりにレビューはせず、自分なりには意図を理解するように心がけ、コードには全て目を通すようにしています。

あまりに無知の場合は、レビュー外で聞くこともありますが、自分がわからない箇所は積極的にコメントで聞くことも意識しています。

最低限の基準は変えない

納期が差し迫っていたり、緊急度が高い修正の場合でも、コード品質を下げて承認することはないように心がけています。

コードの品質低下による速度向上は一時的な前借りであり、将来必ず負債として返ってくるため、常に最低限の基準を守れるように徹底を意識しています。

終わりに

以上、私が普段コードレビューで行っているルーティーンや、大事にしているレビュー観点を紹介させていただきました。
書いていて、完全に徹底できていないものもあるなと感じたので、引き続き意識しながら取り組んでいきます！

PharmaX では、様々なバックグラウンドを持つエンジニアの採用をお待ちしております。
もし、興味をお持ちの場合は、私の X アカウント（@hakoten）や記事のコメントにお気軽にメッセージいただけますと幸いです。まずはカジュアルにお話できれば嬉しいです！