はじめに
エンジニアとして仕事を始めたばかりの頃、「とりあえず動けばOK」と思ってコードを書いていた。
しかし実務でコードレビューを受けると、次のような指摘を大量にもらった。
- 命名がわかりづらい
- 可読性が低い
- 拡張性がない
- テストしにくい
- 実務として不安がある
当時は「動いているのに何がダメなんだ」と感じていたが、今振り返るとすべて正しかった。
この記事では、レビューで実際に指摘されたポイントと、その改善内容、そこから得た学びをまとめる。
① 意味のわからない命名
Before
function calc(u) {
if (u.t === 1) {
return u.p * 0.9;
}
return u.p;
}
指摘されたこと
- 関数名・変数名から意味が読み取れない
- 他人が読めないコードは保守できない
After
const USER_TYPE_PREMIUM = 1;
const DISCOUNT_RATE = 0.9;
// ユーザーのタイプに応じて割引価格を計算する
function calculatePrice(user) {
if (user.type === USER_TYPE_PREMIUM) {
return applyDiscount(user.price);
}
return user.price;
}
// 割引率を適用
function applyDiscount(price) {
return price * DISCOUNT_RATE;
}
学び
- 名前は意味を伝えるためにある
- 命名でコードの理解コストは大きく変わる
- 条件文や処理の背景は必要に応じてコメントで補足する
② マジックナンバー
Before
if (user.type === 1) {
return price * 0.9;
}
指摘されたこと
- 数値の意味が不明
- 仕様がコードから読み取れない
After
const USER_TYPE_PREMIUM = 1;
const DISCOUNT_RATE = 0.9;
// プレミアムユーザーの場合は割引率を適用
if (user.type === USER_TYPE_PREMIUM) {
return price * DISCOUNT_RATE;
}
学び
- 数値には意味を持たせる
- 定数化とコメントで、仕様を明確にする
③ if文ネスト地獄
Before
function process(user) {
if (user) {
if (user.isActive) {
if (user.age > 20) {
return "OK";
}
}
}
return "NG";
}
After
function process(user) {
// ユーザー情報が存在しない場合はNG
if (!user) return "NG";
// 非アクティブユーザーはNG
if (!user.isActive) return "NG";
// 20歳以下のユーザーはNG
if (user.age <= 20) return "NG";
return "OK";
}
学び
- ネストは浅くする
- 条件の意図はコメントで補足
- 早期リターンでコードをシンプルに
④ 1つの関数に詰め込みすぎ
Before
function handleOrder(order) {
if (!order) return;
let total = 0;
for (const item of order.items) {
total += item.price;
}
if (order.user.type === 1) {
total *= 0.9;
}
console.log(total);
return total;
}
After
function handleOrder(order) {
// 注文が存在しない場合は処理しない
validateOrder(order);
// アイテム合計を計算
const total = calculateTotal(order.items);
// ユーザータイプに応じた割引を適用
const finalPrice = applyUserDiscount(order.user, total);
// 処理結果をログ出力
logPrice(finalPrice);
return finalPrice;
}
学び
- 1関数1責務を意識する
- 処理ごとにコメントで補足すると読みやすさが向上
- コメントは「なぜこう書いたか」を伝える手段
⑤ 副作用がわかりにくい
Before
function addItem(cart, item) {
cart.items.push(item);
}
After
// cart を直接変更せず新しいオブジェクトを返す
function addItem(cart, item) {
return {
...cart,
items: [...cart.items, item],
};
}
学び
- 副作用は意識的に扱う
- 条件や処理の意図はコメントで補う
⑥ コメントの書き方で怒られた
Before
// ユーザーの価格を計算する関数
function calculatePrice(user) {
// プレミアムユーザーか判定
if (user.type === 1) {
// 0.9倍割引する
return user.price * 0.9;
}
// 通常ユーザーはそのまま
return user.price;
}
After
const USER_TYPE_PREMIUM = 1;
const DISCOUNT_RATE = 0.9;
/**
* ユーザーの価格を計算する関数
*
* @param {Object} user - ユーザー情報
* @param {number} user.type - ユーザーのタイプ (1=プレミアム)
* @param {number} user.price - ユーザーの元の価格
* @returns {number} 計算後の価格
*
* @description
* - プレミアムユーザー (type=1) には割引率 DISCOUNT_RATE を適用
*/
function calculatePrice(user) {
// プレミアムユーザーには割引を適用する
if (user.type === USER_TYPE_PREMIUM) {
return applyDiscount(user.price);
}
return user.price;
}
/**
* 割引を適用する関数
*
* @param {number} price - 元の価格
* @returns {number} 割引後の価格
*
* @description
* 割引率 DISCOUNT_RATE を掛けて価格を計算
*/
function applyDiscount(price) {
return price * DISCOUNT_RATE;
}
学び
- コメントは「なぜそうしているか」を書く
- 誰が読んでも迷う条件や仕様は必ずコメントで補う
- 「このコメントが消えたら困るか」で判断する
- 個人的には書きすぎなぐらいでいいと思う
⑦ mockを実コードに入れてしまった話
Before
function fetchUser() {
return {
id: 1,
name: "test",
};
}
After
async function fetchUser(apiClient) {
// 実APIからユーザー情報を取得
return apiClient.get("/user");
}
// テスト用のmockクライアント
const mockApiClient = {
get: async () => ({ id: 1, name: "test" }),
};
学び
- mockはテスト側に置く
- 本番コードに混ぜない
- 依存性注入で切り替える
⑧ 「とりあえず」で入れたコード
Before
// とりあえずこれで動く
if (status === 1) {
doSomething();
}
After
// TODO: ステータス仕様が確定したらenumに置き換える
if (status === 1) {
doSomething();
}
学び
- 仮実装は明示する
- TODOやチケットと紐づける
- コメントは「なぜそうしているか」を伝える
⑨ console.logを消し忘れる
Before
console.log("debug:", user);
After
// 処理結果はロガーで管理する
logDebug(user);
学び
- デバッグ用コードは本番に残さない
- ログはロガーや仕組みで管理する
まとめ
レビューを通して理解したのは、「動くコード」と「実務で使えるコード」は別物ということ。
- 読みやすさ
- 変更しやすさ
- 意図の明確さ
- 他人が理解できること
条件文や仕様など誰が見ても迷う部分は必ずコメントで補足する。
おわりに
レビューはきつかったが、「なぜダメか」を理解することで改善できた。
この記事が、レビューで悩む人の参考になればと思う。
他にもいくらでもあると思うので、随時追加していく予定です。