こんにちは！株式会社OPTiMに所属しております川田剣士です。エンジニアとして働き始めてから約1年半が経過し、これまでコードレビューもよく任せていただきました。そこで、コードレビュー時に私が意識しているポイントを皆さんにも共有したく、技術執筆をしようと思いました。記事に対する質問・コメント大歓迎です！
目次
コードレビューを行う目的
コードレビュー時に意識している基本的なこと
・事前に作成した設計通りに実装しているか
・使用していないファイルやコードを残したままにしていないか
・変数名やメソッド名がわかりやすいか
・コードがシンプルかつ一貫性があるか
・プロジェクトのコーディング規約を守っているか
・共通化できるコードをモジュール化しているか
コードレビュー時に意識している技術的なこと
・テストをしっかり書いているか
・エラーハンドリングをしっかり行なっているか
・パフォーマンスの悪いコードを書いていないか
・SOLID原則を守っているか
・ソフトウェアアーキテクチャを守っているか
終わりに
コードレビューを行う目的
まず、そもそもコードレビューを行う目的は何でしょうか？コードレビューの際に意識するべきポイントはあるでしょうが、その前にコードレビューによって何を実現したいか考えることは重要だと思います。そこで、私が考えるコードレビューを行う目的を以下に記載します。
- 求められている要件を満たしているコードであるか確かめるため
- 想定外のエラーも含めて、運用上発生しうる処理パターンを全てコードで網羅しているか確かめるため(安定的にサービスを運用し、何かバグや問題があっても早期に原因特定し、対応できるようにするため)
- コードの品質(セキュリティや処理でかかる負荷、メンテナンス性・テスト網羅性など)をチェックすることで、エンジニアが長期的に問題なくサービスを開発・運用し続けるため
上記が私の思うコードレビューを行う目的です。コードレビューの目的を常に意識することで、より良いコードレビューができるのかなと私は思います。
コードレビュー時に意識している基本的なこと
事前に作成した設計通りに実装しているか
私は事前に設計した内容がコードで十分に反映されているかを最も注意して確認しています。例えば、私のチームではDDDを採用しており、コードを書く前にドメインモデリングをするのですが、その際に作成したドメインモデルをドメインオブジェクトとして設計通りに実装されているかを確認しています。具体的には、ドメインオブジェクトが設計通りにプロパティを保持し、適切にメソッド・バリデーションが実装されているか(要はドメインモデルの性質・振る舞いを十分に実装しているか)を確認しています。また、APIの内部処理に関しても、条件aの場合〜の処理を行い、条件bの場合〜の処理を行うなどの話し合いを行いますが、事前に話し合った処理が実際にコードで実装されているかを確認しています。
事前に設計した内容をコードで十分に反映させるというのは、エンジニアとして必ずやらなければいけないことだと思います。どんなに完璧な設計をしたとしても、設計がコードで反映させていなければ水の泡となるためです。もし、一部の仕様を考慮せずにMergeRequestを依頼してしまう方は、実装時もこまめに設計書を確認したり、仕様の確認のためにチームメンバーとコミュニケーションを取る癖を作ると良いと思います。そうすることで、実装漏れの可能性を最小限に抑えることができると思います。
使用していないファイルやコードを残したままにしていないか
使用していないファイル・コードを残したままにしていないかどうかはかなり重要です。使用していないファイル・コードを残したままにすると、後々改修する開発者が戸惑ってしまい、余計なことで頭に負荷がかかり、時間も浪費してしまいます。使用していないファイルやコードは必ず削除しましょう。
変数名やメソッド名がわかりやすいか
変数名やメソッド名がわかりやすいかはかなり重要です。変数名に関してはどんなデータを格納しているか、メソッド名に関してはどんな処理を行なっているかを一目みてわかるように命名してください。
例を出して説明します。例えば、特定の会社に所属する従業員の中で、指定された年齢以上の年齢かつ最も若い年齢を取得するための関数があったとします。
ダメな例
getMinEmployeeAge(companyId, minEmployeeAge)
良い例
getMinEmployeeAgeFromAgeThreshold(companyId, ageThreshold)
ダメな例の場合、特定の企業に所属する従業員の中で最も若い年齢を取得するのかなと思いきや、minEployeeAgeを引数で渡していて、すでにminEmployeeAgeはわかっているのでは？みたいな疑問が生まれると思います。そのため、getMinEmployeeAge(companyId, minEmployeeAge)が内部でどのような処理を行なっているか実装を確かめる必要が生まれ、余計なエンジニアリングコストが発生します。しかし、getMinEmployeeAgeFromAgeThreshold(companyId, ageThreshold)の場合は、企業のidと年齢の閾値を渡して、従業員の中で年齢の閾値以上かつ最も若い年齢を取得したいのだなということが関数名と引数名のみでわかると思います。(良い例に関しては、もっとより良い命名があるかもしれませんが、そこは甘めに見てもらえると...)
命名に関してはなるべく短くかつ分かりやすいにする必要があります。優先度の高さとしては、分かりやすさ>命名の短さです。多少長くなっても良いので、わかりやすさを重視して命名した方が良いです。
コードがシンプルかつ一貫性があるか
コードがシンプルであることは重要です。基準としては、コメントがなくてもすんなりと頭に入るようなコードが良いです。パフォーマンス上影響を及ぼさない限り、多少長くなっても良いので読みやすいコードを書きましょう。
一貫性に関して、ロジック・実装方針はできる限り一貫性があると良いです。例えば、ある処理Aと処理Bがあるとして、処理Aと処理Bでほぼ同じロジック・実装方針で実装できるにも関わらず、別のロジック・実装方針で実装するという内容です。(処理AとBは共通の処理としてモジュール化しないものとします。)これは、コードを改修するときに、処理Aと処理Bの2つのロジックを理解することに時間がかかってしまいます。できる限り、似たコードは同じようなロジック・実装方針を採用してください。私としては、参考にできそうなコードが既にある場合、コピペ先のコードを理解しているならば、コピペで作成しても問題ないと思います。自力で0から実装すること自体に価値はありません。コードの一貫性を意識しましょう。
ちなみに上記の考え方はKISSの原則(Keep It Simple, Stupid.シンプルにしておけ！この間抜け)と呼ばれています。気になる方は調べてみると良いかもしれませんね。
プロジェクトのコーディング規約を守っているか
開発したコードがコーディング規約を守っているかは重要です。開発チーム全体でシンプルかつ一貫性のあるコードを開発し続けるために、コーディング規約をチーム全体で守る必要があります。コーディング規約のドキュメントは20分もあれば十分読み込むことはできると思います。まだ、プロジェクトのコーディング規約を読んでいない方は、コーディング規約を読みましょう。
共通化できるコードをモジュール化しているか
共通化できるコードをモジュール化しているかどうかは重要です。必要な際にその都度同じコードを実装するのは、ロジックの修正を行う際は全てのコードを同じ様に修正しなければならず、余計なエンジニアリングコストが発生します。また、本来共通化するべき処理を共通化しないと、変更時に整合性が取れなくなる危険性が高まるため、共通化するべきロジックは共通化しましょう。
共通化するべきロジックとは、ユースケースごとにロジックの中身が変わらず、かつ複数回利用されることが容易に想定できる処理です。例えば、商品の消費税の計算をするロジックなどは、ユースケースによってロジックに差分が生まれるわけではないため、1つのモジュール内にロジックを格納した方が良いです。そして、もし消費税の割合などが変更された場合は、消費税の計算ロジックを格納しているモジュールのみ修正を加えれば良いため、修正するべき箇所も明確になり、保守性も高くなります。
ちなみに上記の考え方はDRY原則(Don't Repeat Yourself.同じことを繰り返すな)と呼ばれています。気になる方は調べてみると良いかもしれませんね。
コードレビュー時に意識している技術的なこと
テストをしっかり書いているか
テストをしっかり書くことは重要です。テストのないコードは客観的に見て問題ないかどうかを判断することができないです。基本的にテストはc1カバレッジ100%を目指しましょう。c1カバレッジに関してはいつも忘れてしまうC0/C1/C2カバレッジまとめなどが参考になります。
具体的なポイントとしては、エンドポイントを叩いて実行するテストに関しては、リクエストボディのプロパティが不正の場合・空の場合などをテストしましょう。(認証認可などに関しては、認証認可の責務を負うマイクロサービスがある場合は、e2eテストで確かめて良いかなと思います。)ユースケーステストに関しては、dbが不整合な状態の場合の例外処理など、usecaseの処理内で実装している例外処理は全てテストでキャッチするか確かめましょう。ドメインオブジェクトに関しては、コンストラクタに実装しているバリデーションが正しく機能するか、インスタンスを作成して確かめましょう。また、Repositoryに関しては、全ての分岐をカバーすることを目指してテストを書きましょう。
要は基本的に全ての処理に関してテストを書く必要があります。ただし、特定のテストを作成しようとしたときに工数が大きくなりそうな場合(mockが大量に必要だったり、トランザクションが機能しているか確かめるようなテストなど)は、技術的負債として別のチケットに残すか、あるいはテスト自体作成しないかをMRのレビュワーやチームメンバーと適宜相談した方が良いです。
また、テストを作成する際は同値分割法や境界値分析をすることで、効率的にソフトウェアの品質を調べましょう。
全ての処理をテストで確かめることは理想ではありますが、プロジェクトのスケジュールや開発方針によっても変わると思います。また、テストコードは作成するだけではなく、その後管理する必要があるため、テストを大量に作成するとその分管理コストも増加します。そのため、どのテストは作成し、どのテストは作成しないかに関して、開発チームで共通認識を持った方が良いと思います。(例えば、「Repositoryのsmall testは作成しない」は私の所属する開発チームで定められているテスト作成のルールです。)
テストを意図的に作成しないことは、チーム内で合意をとっている場合は問題ないと思います。ただし、基本的な原則として、実装した処理は全てテストで網羅するようにした方が良いということを頭の片隅に置いておくと良いかなと思います。
エラーハンドリングをしっかり行なっているか
エラーハンドリングをしっかり行なっているかは重要です。具体的には以下の点を満たしているか意識しています。
- エラーが発生した際、お客様からはどのようにエラーメッセージが見えているか(開発者とお客様でエラーメッセージが異なるようになっているか)
- エラーが発生した際、何らかの通知処理が行われているか
- エラーメッセージを見たときに、具体的にどんな事象が起きているかメッセージからわかるようになっているか
エラーメッセージに関して、開発者が見る場合はより詳細な情報があると早期に対応しやすいです。しかし、開発者と同じエラーメッセージをお客様にも見せてしまうと、セキュリティ上リスクが発生します。そのため、お客様には最小限のエラーメッセージを表示させ、開発者サイドには十分な質・量のエラーメッセージをキャッチできるようにしましょう。具体的には、エラーメッセージは勿論、stack traceなどもdatadogなどから確認できるようになると良いです。
また、エラーが起きたことを早期に検知するために、何らかの通知処理を実装しましょう。エラーが起きた時に、メールやTeamsでエラーが起きたことを通知するようにすることで、エラー発生時に早急に対応できるようになります。
パフォーマンスの悪いコードを書いていないか
パフォーマンスの悪いコードを書いていないかは重要です。具体的には
- N+1問題を起こしているコード
- 必要最小限以上にdbにアクセスするようなビジネス・ドメインロジック
などです。1つ目に関しては、私の開発チームではjoinで1クエリで取得しているため、指摘する機会はないのですが、ビジネス・ドメインロジックに関しては余計にdbアクセスをしているようなロジックも見られます。勿論、運用上dbに存在することが想定されるデータ量や本ロジックの想定される利用回数が少なく、かつ可読性の向上が見込める場合は採用する可能性があります。ですが、基本的に可読性よりもパフォーマンスのほうが重要です。ロジックが合理的であるか意識して開発しましょう。
SOLID原則を守っているか
SOLID原則を守ることは重要です。SOLID原則は以下の五つの原則の頭文字を取ったものです。
- Single-responsibility principle(単一責務の原則)
- Open-closed principle(開放閉鎖の原則)
- Liskov substitution principle(リスコフの置換原則)
- Interface segregation principle(インターフェース分離の原則)
- Dependency inversion principle(依存性逆転の原則)
SOLID原則に関しては、責務はどう分離すればいい？良い設計の指針「SOLID原則」を知るという記事がわかりやすかったため、是非ご覧ください。
SOLID原則に関して、細かい説明は本記事では行いませんが、インターフェース分離の原則を実施する意味について以下に記載します。(ここは自分も最近まで理解していなかったため。)
インターフェース分離の原則を実施することで使用しないメソッドをクラスで作成するようなことがなくなります。例えば、親クラス(abstract class)、子クラスA、子クラスB、子クラスCがあったとして、子クラスAでのみ利用されるメソッドを親クラスに定義したとします。そして、子クラスB、子クラスCでメソッドが呼ばれた際は例外を発生させるようにします。この状態はリスコフの置換原則に違反していることになります。サブタイプのオブジェクトはスーパータイプのオブジェクトの仕様に従わなければいけませんが、守られていないためです。インターフェースの分離をすることで、子クラスで利用しないメソッドを親クラスで定義することがなくなり、リスコフの置換原則を守ることができます。
上記がインターフェース分離の原則を実施する意味なのですが、自分はインターフェース分離の原則をコードレビューや設計時に特に意識していないです。また、業務上でも1つのクラスが複数のインターフェースを利用しているようなコードを見たことがないです。理由は、設計が複雑になるというデメリットが発生するためです。本来の正しさでいうとインターフェースを分離したほうが良いですが、実際にインターフェースを分離するかに関しては開発チームで相談すると良いと思います。
SOLID原則に関しては、高凝集・低結合を実現するための重要な原則だと私は捉えています。設計・開発する際はSOLID原則を守っているか常に意識すると良いでしょう。
ソフトウェアアーキテクチャを守っているか
チーム内で採用しているソフトウェアアーキテクチャを守っているかは重要です。私の所属するチームではオニオンアーキテクチャを採用していますが、それぞれの層には責務があります。それぞれの層の責務を理解し、本来実装するべき層に実装されている必要があります。そのため、自分の所属する開発チームで採用しているソフトウェアアーキテクチャに関してはしっかり理解し、一貫性のある設計・開発をしていきましょう。
終わりに
最後まで読んでいただき、ありがとうございます！本記事に対する質問・意見をお待ちしています！