【コードレビュー】かけだしエンジニアの記憶に残ったレビュー7選｜工場プログラマーのモダン開発プラクティス

どうも、コンです。

チーム開発に入ってから、コードレビューでたくさんの指摘を受けてきました。やさしい言い方のものもあれば、一言で心をえぐってくるものもありました。

当時の自分は、フレームワークを理解するだけで精一杯で、「動くコードを書く」ことにやっとでした。そんな状態でレビューを受けると、正しいことを言われているのはわかるんですが、受け止める余裕がありませんでした。

ここでは、そんなかけだし時代に実際にもらった指摘のうち、特に記憶に残っているものを振り返ってみます。

記憶に残ったレビュー 7選
全部並べて気づいたこと
まとめ

記憶に残ったレビュー 7選

1.「i と j に名前をつけてください」

ネストしたループで i と j を使っていました。

// 自分が書いたやつ
for (let i = 0; i < categories.length; i++) {
  for (let j = 0; j < categories[i].products.length; j++) {
    // ...categories[i].products[j] って何？
  }
}

レビュアー：「[SHOULD] ループがネストしている場合は特に、indexに名前をつけてください。i, j, kが何か、わかりにくいです」

categoryIndex、productIndex。言われてみればその通りです。でも正直、最初は「そこ見るんだ」と思いました。アルゴリズムを正しく動かすだけで頭がいっぱいで、変数名にまで気が回っていませんでした。

2.「TopTable という名前はおかしい」

// 自分がつけた名前
const TopTable = () => { ... }

レビュアー：「TopTableという命名はこのテーブルの役割を表しているようには思えません。UI上の表示の位置というのはこのテーブル本来の役割とは独立して変わりうるものだと思います。きちんと役割を表す名前をつけてください。」

画面の一番上にあるテーブルだからTopTable。他の候補は考えませんでした。指摘を受けて、そのテーブルに表示するデータの内容から名前をつけ直しました。

「場所」じゃなくて「役割」で命名するということ自体、このとき初めて意識しました。

3.「filterの前にsortしてますけど、逆です」

// 自分が書いたやつ
const result = products
  .sort((a, b) => a.price - b.price)
  .filter((p) => p.category === selected);

レビュアー：「filterをかけて要素数を減らした後にsortしたほうが計算量減らせてよいです」

結果は同じです。でもsortはO(n log n)、filterはO(n)。先にfilterで絞ってからsortすれば、ソート対象が減ります。「動けば同じ」じゃなかったんです。

計算量という概念は知識としてはありましたが、図の作成や分析にPythonコードを使っていた自分にとってはあまり重要にしていない内容でした。フロントエンドとバックエンドでは大事な視点なんだと知った瞬間でした。

4.「ループの中でのfind（線形探索）は推奨しません」

配列の中から条件に合う要素を探すとき、ループの中で find を使っていました。

// 自分が書いたやつ
for (const order of orders) {
  const customer = customers.find((c) => c.id === order.customerId);
  console.log(customer?.name);
}

レビュアー：「ループの中でのfind（線形探索）は推奨しません。要素数が増えるとすぐに速度のボトルネックになります。常に計算量を意識してコードを書いていただきたいということをお伝えしたいです」

ループの中でfindを回すと O(n × m) になります。先にMapに変換しておけば O(n + m) で済みます。

// 修正後：先にMapを作る
const customerMap = new Map(customers.map((c) => [c.id, c]));

for (const order of orders) {
  const customer = customerMap.get(order.customerId);
  console.log(customer?.name);
}

Mapという選択肢を知りませんでした。この指摘の後、他の場所でも同じパターンをやっていたことに気づいて、まとめて直しました。
自分のコードを検索して同じことをやっている箇所がいくつも出てきたときは、さすがに気まずかったです。

5.「テストは『実装の方法』ではなく『動作結果』を検証すべき」

コンポーネントのテストで、CSSの具体的な値を検証していました。

// 自分が書いたやつ
expect(cell.style.backgroundColor).toBe("rgb(255, 200, 200)");
expect(cell.style.fontWeight).toBe("bold");

レビュアー：「CSSの中身を具体的に書きすぎです。テストが実装の具体に依存しすぎています。テストは極力テスト対象の『動作結果』を検証すべきで、『実装の方法』を検証することは避けなければなりません。なぜなら、実装を変更しづらくなってしまうためです。リファクタリング等で実装を変更する際に、テストも必ず変更しなければならなくなります」

// 修正後：data-testidで要素の存在を検証する
expect(screen.getByTestId("highlighted-cell")).toBeInTheDocument();

当時はテストを書く習慣がなくて、「テストは、とにかくたくさん書けばいい」と思っていました。

CSSの値まで検証していたのは、それが一番確実だと思ったからです。でも結果的に、CSSの色を変えただけでテストが壊れる状態を作っていました。

自分の実装は意味のない努力だったと、レビューで気づきました。

6.「一応コメントはしておきます」

Next.jsを使い始めたとき、まだ仕組みを理解しきれておらず、エラーが出るたびにおまじない的な設定を追加していました。公式ドキュメントを読んだわけではなく、他の実装コードのまねでした。よくわからないけど、つけたら動く。動くならいいか、とそのままPRを出しました。

レビュアー：「一応コメントはしておきます。現在のNext.jsのスタンダードな使い方とは異なります」

「一応コメントはしておきます」の圧。動く理由を理解しないまま出していたことが、完全に伝わっていました。読んだ瞬間、「あ、怒らせてしまった」と思いました。

7.「この変更はなんのためですか？」

あるPRで、本題と関係ないファイルの変更が混ざっていました。ブランチを間違えて書いたコードでした。

レビュアー：「この変更はなんのためですか？」

ブランチを間違えました、としか言いようがないんですが、それがそのまま言い訳にしかならない。このレビューへの返信をどう書けばいいかすら、すぐに出てきませんでした。

ちなみにブランチ間違いは、この一回だけです。慣れましたからね。

全部並べて気づいたこと

7つのレビュー指摘を振り返ると、パターンがあります。

カテゴリ	指摘
命名	#1 i/j、#2 TopTable
計算量・データ構造	#3 sort/filter順、#4 ループ内find
テスト	#5 実装でなく動作結果を検証
設計・その他	#6 スタンダードな使い方との乖離、#7 意図不明な変更