目次
-
- レビューイの注意点1：レビューしやすいプルリクエストを作る
- 1.1 プルリクエストの目的の明示
- 1.2 プルリクエストの分割
- 1.3 コミットの構造化
-
- レビューイの注意点2：コメントを適用する方法
- 2.1 間違ったコメントや質問への対応
- 2.2 提案の意図の理解
- 2.3 他の部分への適用
- 感想
はじめに
最近、チームでコードレビューをする機会が多くなり、レビューに関する本を探していたところ、「読みやすいコードのガイドライン」を見つけた。チームに共有したい内容が多かったため、内容をまとめた。
レビューイの注意点
自分では読みやすいと思っていても、背景知識や思い込み等によって、他の人にとっては読みにくいコードになることもある。そうした事態を避けるためにも、コードレビューを通じて第三者の目でコードを確認し、コードの可読性を検証することは重要。
1. レビューイの注意点1：レビューしやすいプルリクエストを作る
効果の高いレビューを行うためには、プルリクエストの作り方にも気を配る必要がある。
レビューしやすいプルリクエストを作るためには、以下の3点が重要。
- プルリクエストの目的が明確
- プルリクエストのサイズが小さい
- プルリクエストに含まれるコミットが構造的
1.1 プルリクエストの目的の明示
プルリクエストの目的を明示しておくことで、コード変更の理由が分かりやすくなり、レビューを円滑に進められる。必要に応じて以下のようなことをプルリクエストの説明として記載すべき。
- プルリクエストで達成する主な目的
- このプルリクエストで行わないこと ★重要
- 今後のプルリクエストの計画 ★重要
レビューアは基本的に、プルリクエストに改善の余地がある限り、指摘を続ける必要がある。
それらの指摘の中には、プルリクエストの指摘から外れるようなものや、次のプルリクエストで対応予定だったものもある。
→そのため、「そのプルリクエストで行わないこと」や「今後の計画」を明示しておくことが重要であり、レビューの長期化やプルリクエストの肥大化を防げる。
1.2 プルリクエストの分割
巨大なプルリクエストをレビューするには、
- 単純に時間がかかるだけでなく、気を配るべきポイントがおおくなり、レビューの精度の低下を招く
- 構造の根本的な問題を発見した場合、書き直しが必要なコードを大きくなる
- レビューア・レビューイの双方の生産性低下
大きな機能の開発時、それが単一の機能であっても、複数のプルリクエストに分割することが求められる。
＝個々のプルリクエストがマージされた時点では、機能がまだ完成していない状態。
※継続的なインテグレーションの恩恵を受けたり、他メンバーの開発を阻害しないためには、機能が未完成であっても、プロダクトとしてはビルド可能な状態を保つ必要がある。
分割案１：トップダウン方式
詳細な部分は実装せずに、クラスの骨組みだけを作成。
class UserProfilePresenter(
val useCase: UserProfileUseCase
val profileRootView: View
) {
fun showProfileImage() {
TODO(...)
}
fun addUserTag() {
TODO(...)
}
}
分割案2：ボトムアップ方式
部品を実装し、それを使うコードはあとで作成。
使われていないコードがあるとビルドが失敗するように設定されている場合、@Suppressのように警告を抑制しつつ、TODOコメントで今後の計画について説明する必要あり。
@Suppress("unused")
class UserProfileModel(
val userId: Int,
val name: String,
val profileImageUri: Uri?
)
object UserNameStringUtils {
@Suppress("unused")
fun normalizeEmoji(userName: String): String = ...
@Suppress("unused")
fun isValidUserName(userName: String): Boolean = ...
レビューには時間がかかるため、トップダウンとボトムアップの両方の方式を同時に使い、複数のプルリクエストの作成を並行させるのが効率的。
1.3 コミットの構造化
プルリクエストの意図は、そのタイトルやディスクリプションからだけでなく、コミットの並びからも読み取れるのが理想。そのためには、各コミットの責任を明確にし、不要なコミットがプルリクエストに含まれないように気を配る必要がある。
- 不要なコミットの削除
一時的なコードをコミットとして残した場合は、コード削除用のコミットを別途追加するのではなく、プルリクエストを作る前にコミットごと削除すると良い。
※Commit1に対して何かレビューコメントを書いたとしても、Commit3でそのコードが消えてしまうのならそのコメントは無駄になるため。
- コミットの責任の明確化
1つのコミットで複数の変更を行うと、そのコミットの目的を把握しにくくなり、レビューが困難になる。特にコミットメッセージで言及されていない変更があると、誤解の原因となる。
- 1つのコミットで、ある関数を実装しつつ、関連性の薄い別の関数をリファクタリングしてしまうと、そのコミットで何をしたいのか不明瞭になる。
- 1つのプルリクエストで、名前やフォーマットなどの表面的な変更と、本質的なロジックの変更を行う場合、これら2つの変更は別のコミットに切り分けるべき。
- コミットの責務を小さくするためであっても、手当たり次第にコミットを分割すべきではない。レビューしやすいプルリクエストを作るためには、コミットごとに意味がまとまるように、コミットを分割する「軸」を考慮する必要がある。
2. レビューイの注意点2：コメントを適用する方法
レビューコメントは、コードの可読性や正確性、頑健性を向上させるヒントとして有用。しかし、それを鵜呑みにして、単に指示に従えば良いわけではない。
レビューイがコメントを適用する際に行うべきこと。
- 間違ったコメントや質問が起きた理由を考える
- 提案の意図を理解する
- 提案を他の部分に適用できないかを考える
2.1 間違ったコメントや質問への対応
レビューのコメントの内容は、必ずしも正確であるとは限らない。また、コメントの内容は改善の提案とは限らず、不明なことに対する質問などもある。
特にコードの可読性が十分でないと、不正確なコメントや質問が発生しやすくなる。
例：
if (userModel == null) に対して、「nullを比較する理由は何ですか」というコメントが来た場合
- ロジックそのものを変更して、疑問や誤解の余地を減らす
→ nullチェックをアンハッピーパスとして取り扱っている場合は、それをハッピーパスに統合できないか確認する
※「なぜnullをチェックする必要があるのか」という疑問そのものが起きないように、ロジックを変更できることもある。
- コード内の説明で、疑問の回答や誤解の解消を行う
→ 誰もが読めるインラインコメントで書くことで、他の開発者が同じ疑問を持つことを回避でき、結果的に理解しやすいコードとなる。また、変数に説明的な名前を与えることで、このチェックにどのような意味があるのかを説明する。
val isUserExpired = userModel == null
if (isUserExpired) {
...
2.2 提案の意図の理解
レビューコメントとして、コードの改善案を示されることがあるが、その際「何も考えずに提案をそのまま適用する」のは避けるべき。レビューアがその提案で何を達成したかったのかを確認する必要がある。
また、提案されたコードは、改善したい点のみに着目して書かれている可能性がある。つまり、エッジケースやエラーケースの処理や、本題でない部分の構造や命名、前提条件など、本質的でない箇所が簡略化されている場合がある。さらには、提案されたコードがバグを含んでいることもある。
提案されたコードを適用する前に、動作の検証を行、リファクタリングが必要でないか確認する！
2.3 他の部分への適用
コードのある一部分に対して提案が入った時には、他の部分にも同様の改善を適用できないか検討する必要がある。
例：
「nullの可能性を明示するため、@Nullabeというアノテーションを関数の仮引数に追加してください」というコメントが付けられた場合
同じ関数の他の仮引数や戻り値、さらには同じプルリクエスト内で変更された他の関数にも適用できないかと考えるとよい。
また、コメントで指摘を受けた点については、次回以降のプルリクエスト作成時、レビュー依頼前に確認できると理想的。
感想
個人的に、プルリクエストのオープンからマージまでのリードタイムが大きい主要な原因の一つは、プルリクエストの作り方だと思っている。
今までは各メンバーが、上記の内容の共通認識を持たずに独自のやり方でプルリクエストを作っていたため、レビューアがレビューに取り掛かる心理的ハードルが高い状態だったと感じている。
これらの内容をメンバーの共通認識で持っておくと、無駄なやり取りが減りレビューにかかる負担が大いに減るとともに、レビューの質が上がり、デグレ回避や品質向上につながるはずだ。