ソースコードレビュー時に考えていること


最近ソースコードレビューをすることが多くなってきた。レビューを通して他の人の考え方に触れることにより、その考えとの違いから自分自身の考え方に気がつくこともあるし、その考えとどう向き合えばよいのか悩んでしまうこともある。今回はそのことについて少しまとめてみようと思う。なお、この記事では、特定のプロジェクトについてではなく、複数のプロジェクトに共通して感じていることを整理する。

目次

レビュー対象プロジェクトの概要

まずは、レビューするプロジェクトや、プロジェクト内での立場、レビュー方法についてその概要を整理しておく。

レビュー対象となるのはiOSアプリ開発のプロジェクト。開発言語はObjective-CまたはSwift(そのなかでも最近はSwift 3が増えてきている)。新規アプリ開発、既存アプリの改修のどちらの場合もある。どちらの場合も、テストコードは皆無、あるいはテストコードがあったとしてもカバレージがかなり低かったりメンテナンスされていないことが多い。

ソースコードはGitで管理・共有されており、GitHub Enterprise上のプルリクエストやGitLab上のマージリクエストを通してレビューを実施する。プロジェクト内の私の立場は次のうちのいずれかであり、いずれの場合でも、レビュー対象のソースコードを書いている人は遠隔地にいるか、同じオフィスの中でも離れた席にいることがほとんどである。

  • 自分は仕様検討とおおまかな設計のみを担当し、コーディングは別の担当者に完全に任せている。
  • 複数の作業者でコーディング作業を分担していて、自分も一部のコーディング作業を担当している。
  • 自分は仕様検討にもコーディング作業にも関係していない。

レビュー観点

プログラミング言語やiOS SDKの仕様に適した実装がされているかを確認する。また、これまでに保守・改修案件で苦労してきた経験があるため、可読性・保守性について改善すべき(改善できる)場所がないかについては念入りに確認する。実際に動作確認することもあれば、変更部分の差分のみを確認する場合もある。

指摘時のコメントの書き方は人によってさまざまだとは思うが、私の場合は、明らかに問題があったり改善が必要だと感じる場合には質問の形式をとらずに断定的な表現をし、そのかわりになるべく代案を出すことを心がけている。代案を出すのは、私が現状に対して感じている問題点を具体化するとともに、こちらも一緒に考える意思があることを伝えるためである。

以下にいくつか具体的な確認観点を挙げる。これら以外に、仕様に合わない実装やアプリの強制終了、明らかな誤記・誤用などについて指摘することもあるけれど、そういうのは他のレビュアーも指摘するだろうし、今回は省略しておく。また、要改善の場所への指摘以外に「これはいいな」と思った点についてはポジティブなフィードバックをすることもある。

意図を確認する

同じ問題を解決しようと思っても人によってアプローチが異なる場合があるため、コードを読んでいて「ここでは何をしているのだろう?」と疑問に思ってしまうこともある。そういうときにはまず、書き手に意図を確認する質問をする。(少し余裕があれば、自分がコードを読み込んだ上で、その解釈が正しいかを書き手に確認する場合もある。)

具体的には次のような内容のコメントをすることが多い。

  • 処理の意図を説明するコメントを入れてもらう。(座標計算をしている処理については、このようなコメントが特に役立つ。)ただし、変数名やメソッド名のせいでわかりづらいと感じる場合には、まずは次項のように名前の改善を考える。
  • 処理の内容をより理解しやすくするために、変数名やメソッド名を改善できないか検討してもらう。名前の候補をこちらも可能な限り提案する。

メソッド名については、定義している場所では意図がわかるけれども使用する場所ではわかりづらいことがある。そのような場合についても名前の改善提案をすることがある。

安全を心がける

安全に動作するコードであるかを確認することもある。具体的なために次のような指摘をする。

  • クラスのプロパティを必要以上に外部に公開してしまっていないか。アクセス修飾子が適切に指定されていなければ、privateをつけるなどの対応を依頼する。
  • オプショナル型変数の扱いは適切か。nilを扱う必要がなければ、オプショナル型にしないことを提案する。また、オプショナル型の値に対する開示指定(!)時にアプリが強制終了する可能性があれば、防止策を提案する。
  • マルチスレッドでの実行時に、データが正しく更新されない、描画速度が遅い、クラッシュするなどの問題が発生する可能性があれば、それを指摘し防止策を提案する。

適切な抽象化がされているか

DRY原則は次のように表現される1

すべての知識はシステム内において、単一、かつ明確な、そして信頼できる表現になっていなければならない。

この表現の中の「明確な」や「信頼できる」という部分が大切だと思う。というのも、処理やデータ表現が共通化されたとしても、利用時に混乱したり将来の仕様変更時に対応が困難になったりするような実装は、かえって扱いづらいものとなるからだ。

そのような抽象化がされていると感じた場所に対しては、もう少し抽象化を弱めたりインターフェースを改善したりできないか、指摘・提案することもある。

レビュー時の悩み

レビュアーとしてレビューに参加することについて精神的な辛さを感じることがある。

これは「割れ窓の修復」か「正しさハラスメント」か

私は元々、気になったところについては割と遠慮なくコメントをするほうだった。それは、指摘することによりプロダクトがよりよくなるものだ(その指摘から自分の側に発見や学びの機会が発生する可能性もある)と考えに基いていたためだ。ところが、指摘をするとチーム内が変な空気になってしまうような気がするのだ。

指摘の数が多すぎるのか、細かいところにまで指摘をしすぎているのか、指摘時のコメントの書き方がキツすぎるのか、あるいは他の理由によるものなのか……直接フィードバックがあるわけではないので詳しい理由はわからないのだけれど、指摘内容自体に誤りがないために反論しづらい空気を作っている(いわば「正しさハラスメント」2をしている)ということなのだろうか。そうした雰囲気に敏感な一方で、雰囲気を察して勝手に指摘の数を減らしたり許容レベルを下げてしまってよいのだろうかという迷いも自分の中にある。

たぶん、この問題の本質は、どこまで指摘してもよいのかについて明確な基準がないことだ。コードレビューの目的を次のように記述している3のを先日読んだ。ガイドラインを明文化・資料化まではしなくてもいいけれど、共通認識を作っておくくらいはしておかなければならないと感じている。

コードレビューの目的は、ただコードの誤りを修正するだけではありません。重要なのは、チーム全員に同じ知識を共有させること、またコーディングにおいて全員が守るべきガイドラインを確立することです。

その上で、思いやりをもって遠慮なく指摘する4ことができれば、きっとそれがいちばんよいのだろう。

遠隔からのレビューで感じる難しさ

遠隔からコメント欄で指摘をすると、そのコメントの意図が正しく相手に伝わっているかだけではなく、相手に感情レベルでどのように受け取られているかがわからないことで不安に感じることがある。

コメントの真意をこちらに確認することなく推測されているのを見たりすると「直接訊いてよ!」と思う。逆に、個人的にコメントの内容や修正方針の確認・相談をしてくれる人がいるととても嬉しい。こちらからも、レビューのコメント欄での指摘だけをしておいて「こちらは(技術的に)正しいことを指摘しているんだ」とだけ考えているのではなく、相手がどう受け取っているのかを個別に確認するなどのフォローを入れていくことも必要になってくるのかもしれない。このあたりのコミュニケーションのとり方は相手によっても変わってくるところなので、今後もう少しうまくできるようにチャレンジしていきたいところだ。

コメントを残す

メールアドレスが公開されることはありません。 * が付いている欄は必須項目です