この記事は
コードレビューに関するTipsを投稿しよう！ by CodeRabbit Advent Calendar 2025 シリーズ3 14日目の記事です。
はじめに
コードレビュー、してますか？
少なくともこの業界で働くうえでは、される側/する側はともかくその重要性を大いに認識しているかと思います。
時代に取り残された私は昨今のAIを使ったベストプラクティスなどとても書けません。
そのため、AIが台頭する以前に自分が経験した人力コードレビューの中で、しんどかったコードレビューを思い出せる限りで書いていきます。
あるある/ねーよ の視点でお読みいただけると幸いです。
参考程度に、例となるコードをJavaScriptで記載します。
コード及び指摘そのものは実際のものとは異なりますので予めご了承ください。
（特にコードについてはイメージを伝えるためだけに書いているので適当です…）
指摘内容はインラインコメントで例えば下記のように記載しています。
// [mustなどの略語]： 指摘内容
不幸度：★☆☆ （レビュイーがまだ笑って許せるレベル）
指摘内容が誤っている
いただいた指摘そのものが誤っているケース。
多くはレビュアーの仕様に関する勘違いや、記憶がごっちゃになっている場合に発生します。
誤りを伝えて素直に取り下げていただけるのであればいいのですが、人によっては自分の非を認められず逆ギレされることも…
/**
* 税率を適用した金額を計算します
*
* @param {number} rawPrice 税率適用前の金額（単位：円）
* @param {number} taxRate 税率
* @returns {number} 税率適用後の金額
*/
function applyTax(rawPrice, taxRate) {
return rawPrice * (1 + taxRate); // [must]： taxRateは100で割った値を掛けてください
}
taxRate の仕様を誤認して 100で割るべき という指摘をしています。
仕様としては ( 0 < taxRate < 1 )であるため、指摘どおりに修正すると意図した値にならなくなってしまいます。
こういった指摘に対してはコメントで記載しておくとレビュアーの認知負荷を減らすことができそうです。
/**
* 税率を適用した金額を計算します
*
* @param {number} rawPrice 税率適用前の金額（単位：円）
* @param {number} taxRate 税率 (0 < taxRate < 1)
* @returns {number} 税率適用後の金額
*/
function applyTax(rawPrice, taxRate) {
return rawPrice * (1 + taxRate);
}
レビューに付与された略語の基準が怪しい
付与された略語に違和感があるケース。
ここで言う略語は下記のようなものです。
下記のコードの例では客観的には必須ではないはずなのに必須とされているケースを記載しています。
/**
* 指定された回数文字列を反復した文字列を生成します
*
* @param {string} rawText 反復元の文字列
* @param {number} repeatCount 反復回数
* @returns {string} repeatCountだけ反復されたrawText
*/
function repeatText(rawText, repeatCount) {
const array = [];
for (let count = 0; count < repeatCount; count++) {
array.push(rawText);
}
return array.join(''); // [must]: array.join('');は一度変数に格納してからreturnしてください
}
array.join を一度変数に格納するのは must というほどではないと考えられます。
（コーディング規約で定められている場合はともかくとして should や suggest , imo あたりが妥当）
こういった場合は一度チーム内で略語の定義について認識を合わせる必要がありそうです。
そもそもこのコード例ではよほど rawText か repaeatCount が大きくない限りは rawText.repeat(repeatCount) で良さそうですが…
不幸度：★★☆ （レビュイーが若干顔を歪めるレベル）
「何がだめなのか」の情報しかなく、「どうあるべきか」を別途確認する必要がある
NGということしか伝わらないケース。
「コードがどういう振る舞いをすべきか」が明確にわからないとレビュイーは直せません。
下記のコードの例は極端ですが、これでは意図を汲み取りきれずレビュイーが困ってしまいます。
/**
* オプションデータを取得します
*
* @param {object} requestParameters リクエストパラメータ
* @returns {object} レスポンスデータ
*/
async function fetchOptionalData(requestParameters) {
return await fetch('https://example.com/some/optional/data', requestParameters); // [must]: 仕様と異なります
}
仕様が異なることしか伝わらないので、何をどう直すべきなのかをレビュアーに別途確認する必要があります。
（引数が異なるのか？ URLが誤っているのか？ はたまた戻り値の形式が誤っているのか？）
もちろん、仕様と異なっている実装をした開発者にも一定の責任はあります。
一方で、実装中に仕様が変わるような不安定なプロジェクトのように、仕様についてレビュアーとレビュイーで異なっている場合もあります。
上記のようなコメントのようにNGだけを伝えるのではなくどうすればOKになるのかをレビューしてあげるのが良いでしょう。
（その結果として認識相違に気づくケースも有るかと思います）
指摘内容がExcelなど外部にまとめられている
指摘、修正、確認の履歴を表としてExcelなどに記載するケース。
レビュアー、レビュイーともにコードと表とをにらめっこしなくてはならなくてはいけないので大変です。
（管理する側は一覧として確認できるという点で楽なのかもしれませんが…）
また、指摘対象の行番号やコードは変更とともに頻繁に変わっていくため、いずれ追跡が困難になっていきます。
/**
* 与えられた数値の配列の合計値を計算します
*
* @param {number[]} nums 合計値を計算する数値の配列
* @returns {number} 合計値
*/
function calculateTotal(nums) {
return nums.sum();
}
// Excelなどの別ファイルに "[must]: JavaScriptにはArray.sumという関数はないため Array.reduceなどを検討してください" などと書いてある
組織によっては様々な都合から GitHub や GitLab などのホスティングサービスを利用できないケースもあるかもしれません。
であれば Gitea や Forgejo などの導入を検討しても良さそうです。
不幸度：★★★ （レビュイーが頭を抱えるレベル）
担当範囲外のスコープのコードを指摘される
指摘箇所が実装者の担当箇所から外れているケース。
レビュアーによっては実装スコープの範囲を気にせず指摘する方がたまにいます。
そうなると実装スコープはどんどん広がり、いつまで経ってもタスクが終わらない事態に陥ります。
下記の例ではタスクの範囲としては calculateFee の実装だけです。
既存の isWeekend を利用したつもりが、藪蛇のように isWeekend に指摘が入ってしまった例です。
/**
* 与えられた日付が週末（土日）かどうかを判定する
*
* @param {Date} date 判定する日付
* @returns {boolean} true: 週末 / false: 週末ではない
*/
function isWeekend(date) {
const day = date.getDay();
return (day == 0 || day == 6); // [imo]: return (0 < day && day < 6); としても良い気がします
}
/**
* 与えられた日付での運賃を計算します
* 日付が週末の場合割増率を適用します
*
* @param {Date} date 計算する日付
* @returns {number} （休日の場合、割増率を適用した）運賃
*/
function calculateFee(date) {
// 週末の場合は基本料金(BASE_FEE)に割増率(WEEKEND_PREMIUM = 1.2)を乗算する
return isWeekend(date) ? BASE_FEE * WEEKEND_PREMIUM : BASE_FEE;
}
imo であればまだ良いかもしれませんが、 must などで指摘されてしまった日には目も当てられません。
落ち着いて別タスクとして切り出すようレビュアーに進言するのが良さそうです。
指摘内容が小出しのため、ラリーが何度も発生する
レビューのたびに指摘が出るためいつまでもレビューが完了しないケース
レビュアーがレビューのたびに何かを指摘しなくてはならない、という気持ちでいるために発生するのかもしれません。
指摘→対応→指摘→対応→…を延々と繰り返すことになり、（特にレビュイーが）疲弊します。
例えば下記のようなクラス及び、使用する関数を作成したとします。
/**
* プレイヤークラス
* @class
*
* @classdesc このゲームのプレイヤーを示すクラス
*/
class Player {
/**
* コンストラクタ
*
* @param {string} username ユーザー名
* @param {Date} birthday 生年月日
*/
constructor(username, birthday) {
this.username = username;
this.birthday = birthday
}
/**
* ユーザー名が不適切かを判定します
*
* @param {string} username ユーザー名
* @returns {boolean} true: 不適切 / false: 適切
*/
static isInalidUsername(username) {
// ユーザー名が事前に定義した不適切な名前ルール(INVALID_USERNAME_RULE)に合致していれば不適切とみなす
return username.match(/INVALID_USERNAME_RULE/g);
}
/**
* ユーザーにペアレンタルコントロールが必要か判定します
*
* @returns {boolean} true: 必要 / false: 不要
*/
needsParentalControl() {
const today = new Date();
let age = today.getFullYear() - birthday.getYear();
const birthdayOfThisYear new Date(today.getFullYear(), this.birthday.getMonth(), this.birthday.getDate());
if (today < birthdayOfThisYear) {
// 今年の誕生日がまだ来ていない場合
age--;
}
return age < ADULT_AGE;
}
}
/**
* プレイヤーを作成します
*
* @param {string} username ユーザー名
* @param {Date} birthday 生年月日
* @returns {Player} プレイヤー
* @throws {Error} プレイヤー名が不適切な場合
*/
function createPlayer(username, birthday) {
if(Player.isInvalidUsername(username)) {
throw new Error('Your username is invalid. Please consider it again.');
}
return new Player(username, birthday);
}
/**
* プレイヤー名を変更します
*
* @param {Player} player プレイヤー
* @param {string} username ユーザー名
* @returns {Player} プレイヤー名変更後のプレイヤー
* @throws {Error} プレイヤー名が不適切な場合
*/
function renamePlayerUsername(player, username) {
if(Player.isInvalidUsername(username)) {
throw new Error('Your username is invalid. Please consider it again.');
}
return new Player(username, player.birthday);
}
/**
* ゲーム内アイテムが購入できるかを判定します
*
* @param {Player} player プレイヤー
* @returns {boolean} true: 購入可能 / false: 購入不可
*/
function canPurchaseIngameItems(player) {
return !player.needsParentalControl();
}
上記のコードに例えばこんなレビューがついたとします。
/**
* プレイヤーを作成します
*
* @param {string} username ユーザー名
* @param {Date} birthday 生年月日
* @returns {Player} プレイヤー
* @throws {Error} プレイヤー名が不適切な場合
*/
function createPlayer(username, birthday) {
if(Player.isInvalidUsername(username)) { // [should]: isInvalidUsernameはクラスの外からではなくコンストラクタから呼ぶようにしてください
throw new Error('Your username is invalid. Please consider it again.');
}
return new Player(username, birthday);
}
これを踏まえて改修し、再レビュー依頼したところ更にこんなレビューがついたらと考えてみてください。
/**
* プレイヤークラス
* @class
*
* @classdesc このゲームのプレイヤーを示すクラス
*/
class Player { // [ask]: そもそもPlayerをクラスにする必要ありますか？単にオブジェクトで良い気がします
/**
* コンストラクタ
*
* @param {string} username ユーザー名
* @param {Date} birthday 生年月日
* @throws {Error} プレイヤー名が不適切な場合
*/
constructor(username, birthday) {
if (Player.isInvalidUsername(username)) {
throw new Error('Your username is invalid. Please consider it again.');
}
this.username = username;
this.birthday = birthday
}
/**
* （中略）
*/
}
私なら発狂します。「先に言えよ」と…
レビュー完了のタイミングで「もう指摘事項ないですよね？」と念押ししたほうが良さそうです。
番外編
以降はコードレビュー以前の問題で辛いなと感じた出来事です
バージョン管理システムを使っている意味がない
過去のコードをすべて含めている現場に遭遇したときはゾッとしました。
function convertPosition(x, y) {
// 2009.10.01 レビュー反映のためコメントアウト ここから
// const radius = Math.sqrt(x * x + y * y);
// const theta = (Math.atan2(y, x) * 180) / Math.PI;
// 2009.10.01 レビュー反映のためコメントアウト ここまで
// 2009.10.01 レビュー反映のためコード追記 ここから
const radius = Math.sqrt(x ** 2 + y ** 2);
const theta = Math.atan2(y,x);
// 2009.10.01 レビュー反映のためコード追記 ここまで
return { radius, theta };
}
せめて変更の理由が記載されていればまだマシです。
大抵は変更理由は推測の域を出ず、後の保守を行う人間の認知負荷を増大させます。
（往々にしてこういう現場では経緯を知っているエンジニアは退職済みのことが多い…）
上記はまだ追いやすいですが、1つの処理が膨大で複数の変更が加えられているコードを見たときは思わず逃げ出したくなりました。
そもそもコードレビューの文化がない
「まずは作って後から直す」タイプの組織だとコードレビューが軽視されとにかくコードが量産されがちです。
（特にスタートアップに多い傾向？）
結果、「何をやっているのかわからないが、下手にいじるとぶっ壊れる重要な処理」が生まれます。
そして「リファクタリングをしようにも仕様がわからない」ために、レビューが困難である事態が発生します。
終わりに
AIによるPR作成、PRレビューなど、世はまさに大AI時代になりました。
乗り遅れた身としては隔世の感がありますが、最終決定は人の手に委ねられているものと思います。
（少なくともAI台頭以前に開発が始まったプロダクトは、ですが）
私の記事が過去の遺物となる未来を切に願うばかりです。