コードレビューでよく見るNGコード

はじめに

コードレビューをしていると、「なぜこう書いたんだろう？」と思うコードに出会うことがあります。
もちろんミスや経験不足もありますが、実際には「設計の理解不足」「責務の分離ができていない」「読む人の視点が抜けている」といった理由で発生していることが多いです。
この記事では、実務のコードレビューでよく見るNGコードのパターンをまとめます。

① 命名が曖昧

const data = await fetch('/api/v1/users');
const result = process(data);
const flag = check(result);

このコード、何をしているか分かりません。
命名が曖昧だと「コードの意図が伝わらない」「レビューコストが上がる」「バグの原因になる」といった問題が発生します。
例えば

const userList = await fetch('/api/v1/users');
const activeUsers = filterActiveUsers(userList);
const hasActiveUser = checkActiveUserExists(activeUsers);

命名だけで意味が分かる状態が理想です。

② 1つの関数に責務を詰め込みすぎ

async function processUser(id) {
  const user = await fetchUser(id);

  if (!user) return null;

  if (!checkPermission(user)) {
    throw new Error('no permission');
  }

  const formatted = formatUser(user);
  await saveUser(formatted);

  return formatted;
}

この関数は「取得」「チェック」「整形」「保存」すべてを1つでやっています。
こうなると「テストしづらい」「変更に弱い」「理解しづらい」コードになります。
分割すると

async function fetchUser(id) { ... }
function validateUser(user) { ... }
function formatUser(user) { ... }
async function saveUser(user) { ... }

1関数1責務が基本です。

③ ネストが深すぎる

if (user) {
  if (user.isActive) {
    if (user.profile) {
      if (user.profile.age > 18) {
        // 処理
      }
    }
  }
}

ネストが深くなると読むのがしんどく、条件の把握が難しい状態になります。
例えば

if (!user || !user.isActive || !user.profile || user.profile.age <= 18) {
  return;
}

// 処理

ガード節を使うだけで一気に読みやすくなります。

④ 「何をしているか」だけのコメント

// ユーザーを取得
const user = fetchUser(id);

これはコードを読めば分かります。
こういうコメントは冗長でメンテナンスされないことが多いです。
本当に必要なのは

// 外部APIのレスポンスが不安定なためリトライ処理を入れている
const user = fetchUserWithRetry(id);

「なぜ」を書くコメントが価値になります。

まとめ

コードレビューでよく見るNGパターンは下記です。
・命名が曖昧
・責務が混在している
・ネストが深すぎる
・意味のないコメント
これらは特別な技術ではなく、「読む人の視点を持つこと」でかなり改善できます。
レビューされるコードからレビューされないコードへ近づけることが重要です。

一緒に開発できる方へ

弊社では現在、下記のような形でエンジニアとつながりを持てればと考えています。
・フリーランス・業務委託での参画
・副業からのジョイン
・中長期的なパートナーシップ
コードレビュー文化・設計議論・実務水準に共感いただける方は、お気軽にDMください。