レビュアーの心がけについて

さまざまなプロジェクトでレビュアーをしてきた筆者が大事にしている心がけや、レビューの体制について思うこと、重点的にチェックする観点をいくつか紹介したいと思います。

・レビュアーが仕様を把握しているか？


コーディング規約のチェック、危険なコードのチェック、スペルチェックは機械の仕事です。
こういったものは環境を整備して機械にお願いし、意味のあるレビューをしましょう。
もし、レビュアーが変更のもとになった要求仕様を把握していないのであれば、機械が行ってくれる表面的なレビューと変わりなく時間の使い方が誤っている可能性が高いです。
また、例えば開発環境すら持っていないマネージャなど、コードから離れているメンバーが形だけのレビューをすることも全く意味のない時間の使い方です。

・攻撃的になっていないか？


レビューイの人格を否定するような言葉はもとより、口調が強すぎると、言われた方は嫌な気持ちになり、生産性が下がります。たとえ、社内の先輩・後輩の間柄で先輩がレビューをする場合でも、そういうレビューされた後輩は、将来、その彼の後輩をレビューする時に、同じように攻撃的に接して良いと思ってしまうかもしれません。
ましてや、なんの契約関係もない間柄でレビューをする時は、使う言葉をより注意する必要があります。
レビューという行為は主観と客観の曖昧な場所で行われるもので、意見が割れるケースも多々あるのですが、勝ち負けなどはなく、双方で協力して良いコミットを作り上げるための共同作業であるということを忘れてはなりません。

・そのコードの変更点は仕様を満たしていると言えるか？


当然のことですが、まずはこれを満たしているかどうかをチェックしましょう。
仕様を満たしているかどうか不明な場合は、レビュアーが要求仕様の把握をできていない状態か、
難解なコードが書かれているかのいずれかです。
前者の場合は、今一度仕様の把握に努めなければなりません。
後者のケースなら、より可読性の高い代替案をレビュアーが提示するか、将来そのコードを読むことになるエンジニアのためにプルリクエストのコメントや、コードのコメントでフォローしてもらうなりしましょう。
また、関係のない変更が含まれている場合は、その変更の意図を確認しましょう。
「ついで」という言葉で入った無関係な変更が、もしも、その変更に起因する障害が発生した際に、原因の究明を困難にします。「ついで」は、別のチケットで対応してもらうなりしましょう。

・素早くフィードバックしているか？

バグの解消はコードが書かれてから、時間が経過すればするほど困難になります。
逆に言えば、コードを書いた直後に検出できたバグの解消は容易です。
レビュイーはテストも書いて、「これでいける」と思った後に、レビュー依頼をしてくるわけですから、仮にレビューで受けたフィードバックでコード変更をすることはある意味では想定外ですし、優秀なプログラマならば、すっかり気持ちを切り替えてしまって、別の作業に取り掛かっているかもしれません。
レビュー依頼をして、数日経ってから受けたフィードバックによってコード変更が入ると、まずはがっかりしますし、現在進めている他の作業の手を止めて古いプルリクエストの修正作業をしなければならなくなります。
もう一度その古いコミットのコードに向き合うのに、仕様や変更内容を思い出す時間が必要ですし、すぐにレビューされた場合と比べてフィードバックを反映する作業の効率は格段に悪くなります。
「鉄は熱いうちに叩け」とは良く言ったもので、まさにその通りなのです。
レビュアーのタスクの持分を少なくして、レビュアーの手は止まるものの、すぐにフィードバックを返せる体制が望ましいと言えますし、その方がプロジェクト全体としての進捗を押し上げるでしょう。
プログラマー同士でクロスレビューをする文化のあるプロジェクトであれば、他人を思いやって、現在のタスクが一段落したところで（一つのメソッドやクラスの変更作業が終わった、複数ファイルにまたがる同一内容の変更作業が終わったなど）、１両日中にはレビューを終えて、フィードバックを返してあげたいものです。
「このプリリクエストのレビューをお願いしていたんですけど・・・いつくらいに終わりますか？」
という会話が発生するのは情けないものです。

・専門家を巻き込んでいるか？


レビュアーも人の子です。当然わからないこともあるのですが、そういう場合に相談できるDBAなどの専門家を巻き込める力も必要です。持ちつ持たれつという言葉があるように、先方もアプリケーション領域のメンバーに相談したいこともあるでしょう。
レビュアーが良し悪しを判断できない時でも、気楽に相談しあえる文化が醸成された組織は強いと思います。もし、誤った判断でリリースしたバージョンで障害が発生してしまったら、「あの時相談しておけば」と後悔するに違いありません。「後悔先に立たず」です。

・ブランチをチェックアウトして確認しているか？


Githubなどのブラウザで表示されるプルリクエストのdiffだけ見て一目瞭然な簡単な変更でない場合は、必ずブランチをチェックアウトして、IDEでレビューしましょう。
同様の変更をしなければならない箇所が漏れていたり、機械的に変更したがために入ってしまった不要な変更が紛れ込んでいる可能性がありますが、ブラウザで見た時には良いように思える変更も、実際にIDEで見てみるとアラに気づくことが多いものです。

・テストもレビューしているか？


テストも合わせてレビューしましょう。
要求された仕様に伴う変更点が存在するのであれば、その変更内容に即したテストが書かれているはずです。
変更点のテストが追加されており、そのテストが変更点に対して適切にアサーションできているかをチェックしなければなりません。
むしろ、書かれたテストから変更のもとになった要求仕様がより深く把握できるケースもあります。

・コメントもレビューしているか？


「XXテーブルを検索する」などのコメントは自明ですので、除去してもらいましょう。
どういう意図や背景でそのコードを書いたのかに関してコメントが記述されている場合は、将来、保守をするときに有益な情報になります。
そのコメントがわかりやすく、適切な表現で書かれているかどうかも合わせてレビューしましょう。
また、コード変更に伴ってコメントが廃れてしまった場合は、コメントの修正を合わせてお願いしましょう。

・CIが通っているか？


CIが通っていない状態でレビューをすると、レビューをしている最中にコードの変更が入る可能性があります。緊急リリースなどでレビューを同時にしなければならないケースもありますが、通常の開発であれば、CIが通ることを待ちましょう。レビューイもCIが通るまでは、レビュー依頼を待ちましょう。

・コミット単位が大きすぎないか？


レビュアーに負担を掛けている可能性があります。
大きな変更もいくつかのゴールに分けてタスクを分割することで、複数のチェックポイントを設けることができます。
また、分割したチケットのそれぞれで、別のレビュアーを立てることで、複眼的なレビューをすることもできるでしょうし、将来的にはその大きな変更を再び保守しなければならない時に、それに関与した人が多ければ、より有利になるでしょう。

・デバッグプリントやビューに表示されるコメントが残っていないか？


場合によってはシステムをセキュリティリスクを晒すことになりかねません。
特にWebフロントのビュー側のコードにこれが記述されている場合は、レビューでチェックし除去してもらいましょう。

・発行されるSQLが確実に把握出来るコードか？そのコードから発行されるSQLは安全か？


SQLインジェクションが混入するリスクは、現代のORマッパーの進歩によって低リスクになりつつありますが、パラメータをノーチェックでSQLに連結するケースがないとも言えません。コードの複雑度が高ければ、機械的なチェックをすり抜けてくる可能性もありえます。必ずチェックしましょう。
また、WHERE句を動的に組み立てる場合は、レコードが絞られないケースがないことを十分にチェックしましょう。例えば、WHERE句が全く付与されないで10万件のレコードを全てフェッチしてくるようなことがないようにしなければなりません。

最後に

レベル感もさまざまで思いついたことを色々書きましたが、最後に私が最も見ているのはヒトとしてのレビューイです。
決して精神論でこんなことを言っているわけではありません。

  • 過去にこんなことをやらかしたから、ここの部分をもう少し細かく見よう
  • ジョインされたばかりの若いメンバーだけど、ちゃんと要件を把握できていたのかな。もう一回要件を確認してあげようかな。
  • どんなバックグラウンドを持っているのかな。苦手な分野はあるのかな。この言語の経験はあまりなさそうだから、サポートしてあげようかな。
  • 技術的には優れているけど、うっかりミスはないかな
  • このコミットを作るのにどれくらい時間をかけたのかな

何よりもレビューイに寄り添い、そのプルリクエストは同じくらいの責任と思い入れを持って、一緒にコミットを作り上げたと思えるくらい身の入れ方が大事です。
当然レビューの時間は掛かりますし、場合によっては何度もやりとりを重ねるケースもあります。
リリースの時はレビュアーも腹を括り、製品のとって価値のあるコミットが世に出たことを共に喜びましょう。

それでは！