コードレビューの受け方・出し方

指摘されると、自分が否定された気がする

プルリクエストを出すと、次はレビューです。ここで最初につまずくのは技術ではなく気持ちの部分です。前提を 1 つそろえておきます。レビューが検査しているのはコードであって、書いた人ではありません。 同じコードなら誰が書いても同じ指摘が付きますし、10 年書いている人のコードにも普通に指摘は付きます。

この前提は、書く側の言葉づかいで守られます。「この書き方だと分かりにくいです」は人の判断を対象にしていますが、「この条件式は 3 つの否定が入っていて読み解きに時間がかかりました」はコードの状態を言っています。後者なら、読んだ側は防御的にならず素直に条件式を見直せます。主語をコードに置く、これが基本です。

「参考までに」が「必ず直せ」に伝わってしまう

いちばん多い事故は、書いた側が参考程度のつもりだった提案を、受けた側が「必ず直せ」と受け取ることです。強さが伝わっていないだけなので、ラベルで解決します。

種類の多さより、必須かどうかが 1 行目で分かることが大事です。中身の書き方にも型があります。

must: この関数、items が空配列のときに items[0] を参照して
undefined になります。呼び出し元の一覧画面が空のときに落ちるので、
先に length チェックを入れてもらえますか。

何が問題か、どういう条件で起きるか、どうなってほしいか。この 3 点が入っていれば、受けた側は考え直さずにすぐ直せます。良かった点も 1 つ書いてください。指摘だけが並ぶレビューは受ける側にとって消耗する時間ですし、何がチームで良いとされるかの共有にもなります。

40 件のコメントが返ってきて、2 日溶ける

丁寧にやろうとして 40 件のコメントを一度に返すことがあります。内訳は must が 2 件で、残り 38 件は変数名や書式の好み。受け取った側はどれを直すべきか判別できず、全部直そうとして 2 日かかり、本題の 2 件は後回しになります。

原因は量ではなく優先度の欠落です。全体コメントに要約を 1 つ書くだけで、負担は劇的に下がります。

全体としては問題なさそうです。
must が 2 件（空配列の扱いと、エラー時のログ出力）だけ対応をお願いします。
残りは nits なので、気が向いたときで構いません。

もう 1 つの手は、書式の指摘を人がやめることです。命名規則や書式の nits が毎回大量に出るなら、lint の設定で解決すべき問題です。人間が同じ指摘を 3 回書いたら、道具に移す合図だと考えてください。指摘が複数あるときは 1 件ずつ送らずまとめて送ります。1 件ずつだと相手に通知が何度も飛び、そのたびに手を止めさせてしまいます。

直さないなら、理由を書く

指摘に納得して直したときは、対応したコミットのハッシュを添えて返します。レビュアーがどの変更で対応したかをすぐ追えます。

直さない選択もあります。そのときは黙って無視せず、判断とその根拠、そして今後どうするかを書きます。

共通化の提案ありがとうございます。
今は 3 画面それぞれで必要な項目が違っていて、
無理にまとめると条件分岐が増えそうでした。
4 画面目が出た時点で見直したいので、Issue #150 に残しておきます。

これなら提案した側も納得できます。レビューは全部従うゲームではなく、より良い判断に近づけるための対話です。 意図が読み取れないときも、分かったふりをせず「この指摘は〜という意味でしょうか」と聞き返してください。的外れな修正は往復を増やすだけです。