【基本情報技術者の実務】コードレビュー──動くかどうかは、テストの仕事

工房の先輩から、レビューの依頼が届く。

「新人が書いた処理を見てほしいです。同じ計算を3か所にそのままコピーして書いている、変数名が a や b で何の値かわからない、という状態でした。動きはします。本人が萎縮しないよう、良い点にも触れつつ改善点を伝えるレビューコメントを書いてほしいです。」

「良い点に触れてから、提案の形で伝える」で、方向は合っている。基本情報技術者の知識がここで力になるのは、その言い方の手前でレビューが何を見る場なのかを、相手と共有できるときだ。この記事では、コメントの中身と、その並べ方を整理する。

まず結論：動くかどうかは、テストの仕事

レビューを「間違い探し」と考えると、コメントは指摘の量で競うものになる。役割で分けると、置き場所がはっきりする。

今回のコードは「動きはします」という状態だ。つまりテストの側は通っている。ここで見るのは、半年後に別の人が開いたときに読めるかどうかになる。

この線を最初にコメントへ書いておくと、レビューの意味が伝わる。「動いているのに直させられる」ではなく、「動いた後に見る場所がある」という共有になる。

レビューには、いくつかの形がある

形は違っても、共通する原則が1つある。見るのはコードであって、人ではない。指摘は「この計算が3か所にあります」と書き、「3か所にコピーしましたね」とは書かない。主語をコードに置くだけで、同じ内容が受け取りやすくなる。

分かれ目その1：指摘を3段階に分ける

新人が困るのは、指摘の数ではなくどれから手をつけるか分からないことだ。段階のラベルを付けると、コメントは指示ではなく地図になる。

分かれ目その2：その3か所は、本当に同じものか

「同じ計算が3か所にある。1つにまとめよう」は、多くの場合で正しい。ただ、まとめる前に1つだけ確かめたいことがある。

その3か所は、これから先も同時に変わるか。

後者を無理にまとめると、共通化したはずの関数の中に「呼び出し元によって処理を変える」条件分岐が育っていく。読む人は、その関数を読むために呼び出し元を全部たどることになる。共通化の目的は行数を減らすことではなく、直す場所を1つにすることなので、目的から外れてしまう。

今回のようにコピーして貼ったことがはっきりしている場合は、前者だ。まとめる判断でよい。ただしレビューコメントには、なぜまとめてよいと判断したかを1行入れておく。次に似た場面が来たとき、相手が自分で判断できるようになる。

分かれ目その3：名前の長さは、使う範囲で決まる

「a・b は良くない、長い名前にしよう」で終わらせると、次はループの添字まで長い名前になる。基準は文字数ではない。

今回の a・b が読みにくいのは、短いからではなく、使う範囲が広いのに手掛かりが無いからだ。この説明を添えると、相手は「短い名前は全部だめ」ではなく「範囲で決める」という物差しを持ち帰れる。

直す順番：動作を変えない整理と、仕様の変更を混ぜない

提案が通った後の進め方にも、レビューで触れておく価値がある。

1. まず、今のテストが通ることを確かめる。
2. 次に、動作を変えない整理だけを行う（重複をまとめる、名前を変える）。
3. もう一度テストを通し、結果が変わらないことを確かめる。
4. 仕様の変更は、別の作業として行う。

整理と仕様変更を同じ変更にまとめると、テストの結果が変わったときにどちらが原因か分からなくなる。分けておけば、整理の側は「結果が変わらないこと」だけを確認すればよくなる。レビューする側も、差分を読む負担が下がる。

コメントの書き方は、3つで足りる

• 事実：どこに何があるか（「この計算が3か所にあります」）
• 理由：直すと何が良くなるか（「直す場所が1か所になり、直し漏れが起きません」）
• 代案：どうするか（「関数にまとめて3か所から呼ぶ形はどうでしょう」）

質問の形も便利だが、質問だけで終わると相手は何をすればよいか分からない。「ここはどうしてこうしたのですか」と書くなら、その後に代案か、聞いている理由を続ける。

コメントは相手のためだけに書くものではない。半年後、その形になっている理由を探す人が読む。理由を1行残すと、記録になる。

プロならこう伝える

処理を拝見しました。目的どおりに動いていて、テストも通っていますね。ここからは、後で読む人が直しやすいかという観点でお伝えします。動くかどうかはテストが見てくれるので、レビューではそこは見ていません。

【今回直したい】同じ計算が3か所にあります。この3か所は、仕様が変わるときは同時に変わる性質のものなので、1つの関数にまとめて3か所から呼ぶ形はどうでしょう。直す場所が1か所になり、直し漏れが起きなくなります。

逆に、たまたま今の式が同じなだけで、変わる理由が別々のときは、まとめないほうが読みやすいままです。今回はコピーされたものなので、まとめてよい側だと判断しました。

【次でよい】変数名の a・b です。短いこと自体は問題なくて、ループの添字のように数行で閉じているなら i でも読めます。この2つは離れた場所でも使われているので、単価・数量のように中身が分かる名前にすると、読む人が前に戻らずに済みます。

進め方としては、先に今のテストが通ることを確かめてから、動作を変えない整理だけを行って、もう一度テストを通す形が安全です。仕様の変更は別の作業に分けておくと、結果が変わったときに原因を切り分けられます。

今回はどちらも動作を変えない範囲の話なので、1つ目からで大丈夫です。

この場面で効いている基本情報技術者の知識

• 役割分担：動作の正しさはテスト、読みやすさ・直しやすさはレビュー
• レビューの形：ウォークスルー／インスペクション／ピアレビュー。見るのはコードで、人ではない
• 保守性：直す場所が1つになっているか、意図が名前から読めるか
• 共通化の目的：行数を減らすことではなく、直す場所を1つにすること
• リファクタリング：動作を変えない整理。仕様変更とは分けて行う
• 回帰の確認：整理の前後で、テストの結果が変わらないことを確かめる

説明で外したくないポイント

1. 動くかどうかはテストの仕事。レビューは後から読めるかを見る。
2. 主語をコードに置く。「3か所にあります」と書き、「コピーしましたね」とは書かない。
3. 指摘を3段階に分ける。今回直したい／次でよい／好みの範囲。
4. ラベル付けは、自分の点検にもなる。好みの範囲が多い日は焦点がぼやけている。
5. まとめる前に、同時に変わるかを確かめる。
6. 変わる理由が別々なら、まとめない。共通化した関数に分岐が育つ。
7. 名前の長さは、使う範囲で決まる。文字数の問題ではない。
8. 「何と何を区別するための値か」を問うと、名前は決まる。
9. 整理と仕様変更を混ぜない。原因の切り分けができなくなる。
10. 事実・理由・代案の3つで書く。質問だけで終わらせない。
11. 理由の1行は、半年後の誰かへの記録になる。