AIコードレビューの実例で振り返る改善ポイント

はじめに

仕事でコードレビューをしていて、最近増えたなぁと感じるのが、これAIっぽいなぁなコードです。
AIにコードを書かせること自体は、実務ではもう当たり前の前提だと思っています。

以前にも似たようなこと書きましたが、また事例が増えたので書きます。

AIとの共存難しい、、

使えるものはどんどん使えばいいです。
ただ、問題はそのあとで、レビューをしているとかなり似たところで毎回引っかかります。

コードは動くし型エラーもないんです。
開発の仕事でも、

• この分岐、本当に必要？

• 既存の実装を使えない？

• この責務、ここで持つもの？

• そのフォールバック、異常を隠してない？

• その確認、本当にできてる？

と聞いていくと、修正がどんどん出てきます。

「AIでコードを書く力」よりも先に、AIが書いたコードを判断する力が必要になっているとほんとに感じています。
子供でもとりあえず動くものは作れるので、エンジニアの仕事はそこではないってことになりますよね。

私が実際にAIと一緒に実装した機能をレビューしたときに、どんなところに違和感を持ち、どう直していったのかを書き出してみます。

大前提

AIの問題は動かないではない

AIが書くコードは、かなりの確率で動きます。
だから、「動作確認しました」でレビューを終えてコードを見ないと、何も見つからないことがあります。

実際にレビューで出てくるのは、こういうものです。

• 動くけど、要らない

• 動くけど、すでに同じものがある

• 動くけど、このリポジトリの設計と合っていない

• 動くけど、責務の持ち場がおかしい

• 動くけど、本来気づくべき異常を隠している

• 動くけど、ドメインの前提を間違えている

つまり問題になるのは、 「コードとして成立しているか」ではなく、「このシステムに置くコードとして妥当か」 です。
ここはコンパイラも教えてくれません。 人間が判断する必要があります。

具体例

「起こらないこと」への備えを書く

まず一番多いのがこれです。

const title = eventTitle || "";
const startsAt = eventStartsAt || "";
const venue = venueName || "";

一見、安全そうに見えます。 でも型を見ると、

eventTitle: string;
eventStartsAt: string;
venueName: string;

全部必須です。
それなのに、「もし値がなかったら空文字」という処理が入っています。

私は型定義まではまだ見ていない段階でしたがここに対して、「これ違和感しかない。なくていいフォールバックはしないで」と指摘しました。
問題は、単に冗長なことだけではありません。

本来ありえないデータが来たときに空文字で表示してしまうと、異常が隠れます。
本当なら「何かがおかしい」と気づくべきなのに、画面上はただ空欄になってしまいます。
「安全のため」に見えて、むしろバグを見つけづらくしています。

修正後はそのまま使います。

{eventTitle
{eventStartsAt}
{venueName}

そして1箇所見つけたあと、「他にもこういうのある？」と聞くと、同じようなフォールバックが複数箇所出てきました。 AIは、指摘した箇所だけ直して終わりがちです。
1件見つけたら、同じ観点で横展開して探させるのはかなり重要です。

同じ状況への備えが二重になる

こんなコードもありました。

{registration?.participantName ?? "—"}
{registration ? (
  ...
) : (
申込情報がありません。
)}

「申込情報がない」という1つの状態に対して、

• ?? "—"

• 「申込情報がありません」 という2種類の備えがあります。

しかも、そもそもこの画面では、申込情報がない状態に到達しません。
こういうときに私がよく聞くのは、「この分岐、どんなデータのとき通るん？」です。
説明できなければ、その分岐が本当に必要なのかを疑います。

型だけ見て、ドメインのライフサイクルを見ない

イベント画像の表示に、こんな分岐がありました。

{event.coverImageUrl && (
  <Image src={event.coverImageUrl} ... />
)}

私は、「この分岐いる？ 申込できるイベントで画像がないことってある？(ない確信がある)」と聞きました。

するとAIは、「型の上では null になりえます。開発用データにも画像なしのイベントがあります」という根拠を出してきました。

かなりもっともらしいです。
でも、ここで見ていたデータは下書き状態を含んでいました。
実際のライフサイクルを確認すると、

• 下書き保存時は画像なしでも保存できる

• 公開時には画像が必須

• 申込受付を開始するときにも条件を満たす必要がある

という仕組みになっていました。
つまり、 申込できる → 公開済み → 公開時に画像必須 → この画面に来るイベントには画像がある、なければこの手前で落ちてるという不変条件が成立します。

AIが間違えたというより、見えている型とデータだけでは、その前提まで分からなかったわけです。
ここで大事なのは、 「型ではnullだから」で終わらせず、 「その状態は、実際の業務フローの中で発生するのか？」 まで見ることです。

AIはもっともらしい根拠を出してきます。
だからこそ、「それを保証しているコードはどこ？」まで確認します。

「既存コンポーネントを使えない」という説明を検証する

AIが、すでにあるものとほぼ同じ表示コンポーネントを新しく作っていました。

理由を聞くと、「既存コンポーネントが受け取る型には id が必須です。今回のデータには id がないので使えません」とのことでした。
そこで中身を見ると、その表示コンポーネント自身はidを使っていません。

私は、「表示で使ってないなら、id 要求しなくてよくない？それを使って送信するとかそういう処理もないし」という意図で指摘しました。
ただし、ここで私の最初の指示をそのまま実行することはできませんでした。
元の型の id は、別の画面では実際に使われていたからです。

つまり、

type Participant = {
  id: string;
  name: string;
  email: string;
  companyName: string;
};

から単純に id を消すと、別の機能が壊れます。
そこで最終的には、型を責務ごとに分ける形になりました。

// 表示に必要な項目だけ
type ParticipantDisplay = {
  name: string;
  email: string;
  companyName: string;
};

// 管理側では識別子も必要
type Participant = ParticipantDisplay & {
  id: string;
  isDefault?: boolean;
};

表示コンポーネントは ParticipantDisplay を受け取る。
一覧や編集では Participant を使う。
こうすると、既存機能を壊さずに、「表示だけのコンポーネントに、使っていない id を要求させない」という目的を達成できます。

結果、新しく作っていた表示用コンポーネントも削除できました。

大事だったのは、「使っていない値を、このコンポーネントに要求してほしくない」という意図です。 AIに修正を依頼するときも、「これを消して」だけではなく、「このコンポーネントに使っていないものを要求させたくない」まで伝える。
そうすると、指定した手段が使えなかった場合でも、別の方法を探しやすくなります。

自分の最初の修正案が正しいとも限りません。
レビューでは、手段より意図を伝えることも大事だと感じました。

食い違いを変換関数で埋める

これはほんとにうんざりするAIの挙動ですw
既存コンポーネントを使えるようにしたあと、AIはこんな変換関数を書きました。

const toParticipantDisplay = participant => ({
  name: participant.name,
  email: participant.email,
  company: participant.companyName,
});

(分割代入してくれと思いつつ)

実質的に意味があるのは、company: participant.companyName だけです。 片方では company、片方では companyName と呼んでいるため、変換関数で吸収していました。

私は、「この関数いらん。直接渡せばええやん」と指摘しました。
調べると、companyを使っていたのは、まだ本番APIにつながっていない仮実装でした。
それなら、仮実装側を実APIに合わせる方が自然です。

AIは食い違いを見ると、「間に変換を入れて両方残す」という解を選びやすいです。
でも橋を架ける前に、「そもそもどちらかを合わせられない？」を見る方がいいことがあります。

自分が書いたコードの中の重複を放置する

こんなJSXが並んでいました。

参加費
...
オプション料金
...
手数料
...

人間なら、2回目か3回目くらいで「これまとめられそう」と思います。
AIは一気に生成するので、同じ構造を何度書いたかに気づきにくいことがあります。

私は、「ここスタイル繰り返しあるけど、既存のコンポーネントで使えそうなのない？」と聞きました。
順番も重要です。 いきなり共通コンポーネントを作るのではなく、

1. 既存を探す

2. 本当に使えない理由を確認する

3. なければ新しく切り出す

という順で考えます。
今回は似た既存コンポーネントがありましたが、レイアウトの意図が違ったため、新しく切り出しました。

フォームの責務に取得データまで混ざる

フォーム用のフックが、こんな返り値になっていました。

return {
  register,
  errors,
  isSubmitting,
  registration,
  errorMessage,
  handleSubmit,
};

register, errors, isSubmitting, handleSubmit はフォーム管理の責務です。
でも registration は、APIから取得したドメインデータです。
種類が違います。

私は、「registration返してるのおかしいな」と指摘しました。 こういう責務の混ざりは、返り値を並べて見ると結構分かります。

結果的に、

• データを照会して保持するhook

• 入力フォームを管理するhook

に分けました。

AIは「1機能 = 1フック」のようにまとめて作り、その後増えた責務も同じ箱に追加していくことがあります。
動くので気づきにくいところです。

一時的な通知のためだけにstateを持つ

エラー文言をstateとして持っていました。

const [errorMessage, setErrorMessage] = useState<string | null>(null);

でも、この値は一度画面に出すだけで、後から判断に使うわけではありません。
私は、「このステートもなくていいかも。トーストでいい」と指摘しました。
結果、toast.error(...) だけで済みました。

AIは「表示するもの → state」と対応させがちです。
でも、一時的な通知まで必ずstateとして持つ必要はありません(UIにもよるけど)。

propsでフォーム情報をバケツリレーする

フォーム内で、

<FormFields register={register} errors={errors} />

と渡していました。
このプロジェクトには、すでにフォーム情報をContext経由で扱う仕組みがあります。
増えてきたらContextを使う設計方針だったので、「このprops嫌だな。useFormContext使おうぜ」と指摘しました。

ただし、ここも「Contextにすればいい」で終わりではありませんでした。
既存の共通フォーム実装には少しクセがあり、正しいフォームインスタンスを渡さないと、内部で別のものを生成する仕組みになっていました。

そこを確認せずに書き換えると、「型は通るのに入力が正しく連動しない」という壊れ方をする可能性がありました。

既存の仕組みに合わせるときも、「前例があるから同じにする」だけではなく、その仕組みがどう動いているかまで確認する必要があります。

リクエストボディに型を書かなくても通る

こんなコードもありました。

body: JSON.stringify({
  token,
  ...values,
});

私は、「リクエストボディの型は明示的に当てて」と指摘しました。 例えば、

const body: RegistrationLookupRequest = {
  token,
  ...values,
};
body: JSON.stringify(body);

のようにします。
JSON.stringify() は何でも受け取るので、ここに型を書かなくてもコンパイルは通ります。
だからこそ、型を書かないままだと送信側とAPI契約がズレても気づきにくい場所になります。

同じことはレスポンス側にも言えます。
fetch 周辺は、送る側・受ける側ともに型の穴になりやすいので注意して見ています。

「良い習慣」を過剰適用する

AIがエラーメッセージを定数化していました。

export const LOOKUP_FAILED_MESSAGE = "...";
export const LOOKUP_UNAVAILABLE_MESSAGE = "...";

しかも、この2つだけのためにファイルが増えていました。 私は、「わざわざ要る？ インラインでよくない？」と指摘しました。

定数化自体が悪いわけではありません。 ただ、

• 使用箇所が1〜2箇所

• 値を見るためだけに別ファイルを開く必要があるなら、むしろ読むコストが増える

AIは、定数化・型エイリアス・コメント・網羅的な分岐のような「一般的には良いとされること」を、文脈に関係なく適用することがあります。 良い習慣も、量と場所次第です。

読めば分かることをコメントに書く

こんなコメントもありました。

{/* 照合に失敗したときはトーストで知らせる。 フォームと入力値はそのまま残るので、 打ち間違いならそのまま入力し直せる */}

これはコードを読めば分かります。こういうコメントは削除しました。

一方で、残す価値があるコメントもあります。
コメントに残したいのは、「何をしているか」より「なぜそうしているか」です。

書きすぎる一方で、本当に必要なエラー処理は抜ける

ここまで、不要な防御をかなり削っています。 でも逆に、本当に必要な防御は抜けていました。

const response = await fetch("/api/...");

通信自体は失敗します。
ネットワーク断などで fetch() がrejectするケースがあります。
私は、「fetchはtry-catchいるやろ」と指摘しました。

AIは正常系の筋書きに沿ってコードを組み立てるため、こういう失敗が抜けることがあります。
特にイベントハンドラ内の非同期処理では、例外がそのまま画面のエラー表示に流れてくれるとは限りません。
その場合ユーザーから見ると、「ボタンを押した → 何も起きない」だけになります。

不要なフォールバックはたくさんあるのに、必要な異常系は抜ける。
ここもレビューでよく見るポイントです。

「確認しました」をそのまま信じない

これが一番危なかったかもしれません。
途中でAIから、「ブラウザで動作確認済みです」という報告がありました。

でも実際には、開発環境側でページ全体のJavaScriptが正しく動いていない状態でした。
フォームだけではなく、別のUI操作まで反応していませんでした。
つまり、「動作確認済み」という報告自体が嘘だったわけです。

ここで重要なのは、「確認した？」では足りないことです。
「何を見て確認した？」まで聞きます。

• Network

• Response

• Status Code

• Console

• イベントが実際に発火しているか

• 周辺UIが正常に動いているか

「確認しました」は結論です。
レビューで欲しいのは、その結論に至った観測結果です。

レビューの3つのやり方

ここまで個別のコードについて書いてきました。
振り返るとレビューの進め方にも共通点がありました。

1件見つけたら「他にもある？」と聞く

AIは、指摘された箇所は直します。
でも、同じ問題が別の場所にもあるかまで探してくれるとは限りません。
1つ見つけたら、「他にも同じパターンある？」まで聞く。これだけでレビュー効率はかなり上がります。

説明そのものを検証する

「型が合わない」「データに該当ケースがある」「動作確認済みです」という説明が出てきても、調べると前提が違っていることがありました。
AIは説明もそれらしく作れるので、説明できた＝正しいではありません。
その説明もレビュー対象です。

手段もだけど意図も伝える

指示通りにいかないときも、「なぜ直したいのか」という意図まで伝えると、別の筋の良い設計にたどり着きやすくなります。
レビューするとき、まず持っておきたい3つの問い 全部覚えるのは無理ですが、この3つでかなりの部分が引っかかります。

1. 「この分岐、どんなデータのとき通る？」 不要なフォールバックや、存在しない状態への分岐が見つかります。

2. 「同じことをしている既存コードはどこ？何件ある？」 車輪の再発明や、その場所だけ浮いている実装が見つかります。

3. 「それ、実際に何を見て確認した？」 「確認済み」をそのまま受け取らず、観測した証拠を確認します。

問い方だけ覚えてもレビューはできない

「この3つだけ覚えればいいのでは？」と思うかもしれませんが、そうではありません。
例えば画像の件も、ドメインの流れ（公開処理を通っているはず）を知っていたからこそ違和感を持てました。

レビューには、言語・フレームワーク・型・API・DB・設計・既存コード・ドメイン知識が必要です。 知識があるからこそ、「それ本当？」という問いが出てきます。

AIを使うなら、コードの責任は自分が持つ AIを使えば、自分ではまだ書けないコードも生成できます。 でも、書けることと責任を持てることは別です。

今回直したものの多くは、構文エラーではありません。型も通り、ある程度動きます。 その上で、「この分岐は本当に存在するのか」「この責務はここなのか」「設計と合っているか」を判断して修正しています。

もし分からないコードのままPRに出しても、出したら自分のコードです。障害が起きたときに「AIが書いたので分かりません」は通りません。
AIを使うなら、少なくとも「なぜこのコードになっているのか」を自分で説明できるところまで理解する。そこは人間側が持つべき最低限の責任だと思っています。

最後に

AIにコードを書かせるのは、サボるためじゃなくて人間がもっと大事な設計や仕様の判断に時間を使うためだと思います。
だからこそ、AIが書いたコードの責任を持てるだけの力は鍛えていきたいよね、という話でした。

AIがコードを書いてくれる時代になったからこそ、「書けるか」より「判断できるか」の価値は、むしろ上がっていると思います。

、、、、なんですけど、私もAIの出したコードへの修正箇所が多すぎて(多分変なPRがマージされて変なのが結構混じってるせい)、これ効率化できてるのか？って思いますw