はじめに
この記事について
以下の Toggeter のまとめを読んでブコメしたんですが、他にも思うところがあったのでまとめておきます。
論点
- レビュアーは質問しただけなのにレビュイーがそれに回答せずに修正してしまう問題
- レビュアーは質問の体で暗に非難を込めていないか
- レビュアーはレビュイーが書いたコードの意図を推測できるならその推測に基づいて提案してもいいのはないか
- レビュアーが質問しただけなのにレビュイーが回答せずに修正してしまったらどうするか
前提
コードレビューの目的
一言でいうとこれです。
The primary purpose of code review is to make sure that the overall code health of Google’s code base is improving over time.
The Standard of Code Review | eng-practices
理想は各人がコードのオーナーシップを持ち、コードの健康状態を良好に保つ努力をしてほしいですが、能力や意欲の差がどうしてもあるので、すべての人に求めることはできません。しかし、チーム全体の方向性としてはそっちを向いていきたいので、目的の宣言だったり共有だったりっていうのは大事かなと思います。
人体でもそうですが、どうすれば健康状態を保てるか、というのは運動だったり食事だったり色んな要素があるわけで、コードレビューにおいてもいくつかの要素があると思います。
パッと思いつくものは以下の2つです。
- 不具合に関わること（サービスが止まるような致命的なもの、ユーザーに著しく不快感・不便感を与えうるもの、といったグラデーションがある）
- 保守性に関わること（複雑で修正できなくなる、修正時の影響範囲が広くなっていく、など）
とくに不具合に関することは、
コードレビューのルール
自分の場合はいつも以下のルールでやってます。
- チームにコードレビューのルールやガイドライン（以下ルール）が存在する場合はそれに従います
- チームのルールがない場合、あるいは既存のルールとコンフリクトしない場合は独自のルール（マイルール）を使います
- マイルールはチームのドキュメント管理ツールなどで公開します
場合によっては、マイルールがチームのルールとして採用されることもありますし、自分の判断で真似する人が出てくることもありますし、まったく相手にされないこともあります。チームによりけりですね。
マイルールはどんなものか
ざっくりですが、自分がどんなルールでコードレビューをしているか、前提条件として書いておきます。実際はもっと構造化された文書ですが、簡単のために箇条書きにします。
以下のルールでラベルをつけています。
- [Must] 修正されるまで Approve しません。明らかなコーディングミス、対応する自動テストがない、セキュリティリスクがある、など
- [Should] 原則修正してほしいですが、合理的な理由と思えれば Approve することもあります。標準関数や既存のライブラリ・モジュールを使えばできる処理を自作している、早期リターンを使わず多段ネストしている（目安はブロックが3以上）、判別できない変数名（長さとかは静的解析ツールで捕らえますが、意味が通らない variable_abc みたいなのは検知できない）、など
- [Could] 修正するかどうかはレビュイーに委ねます。自分はこっちのほうがこういう理由でいいと思いますが、どうでしょう？みたいな提案がメインです
- [Question] 単なる質問です。修正の必要はありません。
- （ラベルなし）その他コメント。「この書き方知りませんでした、いいっすね」とか「ついでに直してもらって助かります、ありがとうございます」とかです
できるだけ個人的な選好を排除し、メリット・デメリットを考慮して（コメントにも記載して）書くようにしていますが、たまには個人の好みが出ちゃってるかもしれないです。ただ、現実問題として、好みの問題と良し悪しの問題はなかなか明確な分離が難しいので、あまり厳密に考えないようにはしています。相手が自分の提案に乗ってくれればそれはそれでいいですし、仮に乗ってくれない場合には基本的には自分から取り下げるようにしています。たいていのケースでは、メリット・デメリットはトレードオフになりますし、保守性の観点からの指摘は将来の不確定性みたいなのもあるので、自分の提案が絶対的に優れているということはないように思います。
さて、前置きが長くなりました、以上が前提で、ここから本題です。
1. レビュアーは質問しただけなのにレビュイーがそれに回答せずに修正してしまう問題
自分にも経験ありますが、純粋に疑問に思ったことを尋ねただけなのに、相手がそれを批判と捉えてこちらが希望していないにも関わらず修正を加えてしまうというのは、冒頭の記事についたブコメを見るかぎり、わりとよくあることなのかな、という印象です。
以前は自分も質問だけコメントすることは普通にあったので、この問題には気づいていましたが、それほど深刻には考えていませんでした。
いくつかケースがあって、それぞれで対応が変わってくると思いますが、いずれにしても、あまりいいコミュニケーションとは言えないです。
- 修正してもらっても構わないケース
- 修正されると困るケース
1 に関してはまぁいっかとも思うのでそのままスルーしてましたが、問題は 2 で、意図を勝手に解釈して修正した結果、間違ったコードになってしまうと本末転倒で、そういうときは元に戻してほしいと伝えなくてはならず、自分も相手も時間と労力をロスすることになりますし、あまりいい気分にはなれません。
2. レビュアーは質問の体で暗に非難を込めていないか
振り返ってみると、質問の体で本当に伝えたいことは修正依頼や提案だった、ということは何度かあったように思います。そういうときに感じていたことは、相手のスキルの低さに対する苛立ちだったり、コーディングスタイルが既存のものと違いすぎて受け入れられないという気持ちだったり、いずれにしてもあまりポジティブな記憶はありません。
もちろん、本当に純粋に疑問に思ったことを確認したいということもあるので、確認したあとどうしたいのか、というのが重要で、コードから意図が汲み取れない場合は「コメントで補足してほしいです」とか、なにか事情があってトリッキーなことをしている場合は理由が知りたいとか、その都度考えていけば、純粋に質問のみしたいケースは減っていくんじゃないかと思います。
純粋に質問したいだけなら、レビューコメントに残さずに、Slack で直接聞くこともあります（その回答次第でコメントが変わりそうな場合とか）。
3. レビュアーはレビュイーが書いたコードの意図を推測できるならその推測に基づいて提案してもいいのはないか
そういうわけで、疑問に思ったこと箇所について、もし相手の意図を推測できるなら、その推測に基づいて自分から修正依頼や提案をしてもいいんじゃないかと思って、最近はそういう書き方をするようにしています。
このケースの場合は、たいてい上のマイルールにおける [Should] のラベルがつくことが多い印象です。
チームに入ったばかりのメンバーで、既存コードの理解が浅く、同じ機能を提供する別のモジュールの存在を知らなくて別途実装してしまった、みたいなのはその典型です。
なにか特別な理由があるかもしれないので、こんな感じで質問、推測、提案をセットでします。
[Should] この処理は X クラス（実際はリンク）に同様のことができる Y メソッドがありますが、今回新たに実装したのってなにか理由がありますか？もし知らなかっただけなら X.Y を使ってほしいです。別で実装したほうがいい理由があればコメントに書いておいてください。
他にも、
[Could] このテーブル論理削除になってますが、現時点では YAGNI って気もするんですが、どうでしょう？論理削除にした理由があれば教えてください、とくにないのであれば必要になったら追加するでもいいかと思います。
みたいな、レビュイーに判断を委ねる場合でも、「これってなんで論理削除するようになってるんですか？」だけで終わらず、提案も含めるようにしています（ちょっと例が良くないんですが、個人的にはこの内容であれば [Should] にします、論理削除は最終手段にしたいです。そもそも、テーブル設計のみ先に別ブランチでレビューするようにしたいですが、そこは現場によります）。
4. レビュアーが質問しただけなのにレビュイーが回答せずに修正してしまったらどうするか
上に書いたように、修正してもらっても構わないケースであればそのままスルーでもいいわけですし、修正されると困るケースっていうのもまれなのかなという気もします。
自分としては、マイルールを公開した上で [Question] タグをつけ、万一修正されてしまった場合は都度々々修正は不要である旨説明するでしょう（幸い、まだそういう機会は訪れていませんが）。
おわりに
コードレビューは本当に難しいので、できるだけ Google などが公開しているガイドラインを踏襲しつつ、アップデートしながら、メンバーとコミュニケーションを取りながら最適化し続ける必要があるかな、と思っています。
