1
1

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

AIを使わずにじっくりコードレビューをしたら、指摘する場所が変わってた

1
Posted at

みなさん、エンジニア楽しんでいますか?

私は業務として、初学者の方のコードレビューをする機会が多いです。
ある時ふと、「最近は業務時間にあんまプログラム書いてないし、メインFWのLaravelのスキルアップが止まっているな」と感じました。
プライベートの時間にはそれなりに勉強しているのですが、最近はめっきりGoの学習に費やしています。

そこで、レビューでスキルアップできないかと思い、AIを使わずにレビューしてみることにしました。
やってみると、指摘する場所が以前と明確に変わっていることに気づきました。

この記事で話すこと

  • 「AIを使わない」の定義
  • 減った指摘・増えた指摘
  • レビューされる側に求めるものが変わった話

前提

私は初学者・未経験者の教育をしているので、開発現場のみなさんよりも業務のスピード感はゆったりしています。
スピード重視の現場でAIなしレビューを回すのは正直難しいと思います。

ただ、AI時代にレビューで見るべき場所はどこかという話は、スピード感に関係なく共通する部分があるはずです。
また、本記事のプログラムはLaravel中心です。
Laravelを触ったことがない方向けに、コード内にコメントを入れています。

もう1点、初学者の方の環境には CLAUDE.mdAGENTS.md のような、プロジェクトの規約をAIに読ませる設定ファイルは無いものとして読んでください。
つまり、AIはプロジェクトの方針を知らない状態でコードを出力しています。
このあたりが整備されている現場であれば、後述する指摘のいくつかはそもそも発生しないはずです。

「AIを使わない」の定義

この記事で言う「AIを使わない」は、差分やコードをAIに投げて「レビューして」と指摘を出させることを指します。
公式ドキュメント・リファレンスを読む、実際にローカルで動かす、自分で差分を読むは自分で行います。
「調べもの禁止」ではなく、指摘をAIに任せないという意味です。

結論

やった価値はありました。
理由は3つです。

  1. 月並みだが、AIが書いたコードには、自分が知らない記述が普通に混ざっている。
  2. 指摘対象が「動くかどうか」より「過剰かどうか」が増えた。
  3. レビューを機械的・無機質に捌く発想がなくなり、丁寧に読む時間が増えた

減った指摘

まず、明確に減ったものから。

  • N+1問題: ある程度のプロンプトが書ければ、AIが最初から with() (イーガーローディング)を入れてくる
  • 命名規則・記法の細かいブレ: Laravelの規約に沿った形で出力される
  • バリデーション漏れ: FormRequestごと生成されるので、抜けが少ない

このあたりは、以前なら初学者のプルリクで必ず1回は指摘していた領域です。
今はほぼ出てきません。

裏を返すと、レビュアーが「知識の量」で価値を出す部分は縮んでいるということでもあります。

増えた指摘

1. 過剰実装・不要なテストケース

一番増えたのがこれです。

AIは「テストを書いて」と言われれば書けるだけ書きます。
ただ、初学者の方はその出力が妥当かどうかを精査できないので、そのままプルリクに混ざってきます

たとえば、スレッド投稿フォームに対して、こういうテストが出てきます。

// 【テスト1】title を空で送信したときに、エラーになることを確認する
//
// actingAs()               : 「このユーザーでログイン済み」の状態を作るヘルパー
// post('/threads', [...])  : 第2引数の値をフォーム送信としてPOSTする
// assertSessionHasErrors() : 指定した項目でバリデーションエラーが出たことを検証する
public function test_title_is_required(): void
{
    $response = $this->actingAs($this->user)
        ->post('/threads', ['title' => null, 'body' => 'body']);

    $response->assertSessionHasErrors('title');
}

// 【テスト2】title に数値を送信したときに、エラーになることを確認する
// 「文字列であること(string ルール)」が効いているかを見ている
public function test_title_is_string(): void
{
    $response = $this->actingAs($this->user)
        ->post('/threads', ['title' => 123, 'body' => 'body']);

    $response->assertSessionHasErrors('title');
}

// 【テスト3】title が255文字を超えたときに、エラーになることを確認する
// str_repeat('a', 256) で境界値ちょうど1文字オーバーの文字列を作っている
public function test_title_max_255(): void
{
    $response = $this->actingAs($this->user)
        ->post('/threads', ['title' => str_repeat('a', 256), 'body' => 'body']);

    $response->assertSessionHasErrors('title');
}

3本とも、required / string / max:255 というバリデーションルールが1本ずつ効いているかを確認しています。

一見ちゃんとしていますが、この3本が確認しているのは「Laravelのバリデーション機能が仕様どおりに動くこと」だけです。

requiredmax:255 は、Laravelが標準で用意しているルールです。
これらが正しく動くことは、Laravel本体のテストですでに保証されています。
つまり「requiredを指定したのに空のまま登録できてしまった」という不具合が起きたなら、それは私たちのアプリのバグではなく、Laravel側のバグということになります。
自分たちのテストで守る必要はありません。

では何を守るべきかというと、そのアプリでしか成立しないルールのほうです。
たとえばスレッド投稿機能なら、こういったものが該当します。

  • 同じユーザーが、同じタイトルのスレッドを2つ作れない
  • 公開中のスレッドは削除できない
  • 下書き状態のスレッドは、作成者本人にしか見えない

いずれも仕様として決めて、自分たちで実装した判断です。
本記事ではこれを「アプリケーション固有のロジック」と呼んでいます。
フレームワークは何も保証してくれませんし、実装を変えれば自分たちで壊し得るので、テストで守る価値があります。

先ほどの3本には、この観点が1つも入っていません。
加えて、バリデーションルールを1つ増やすたびにテストメソッドが1つ増えるので、FormRequestを修正するときの負債にもなります。

なので、こういう指摘をするようになりました。

このテスト、バリデーションルールが効いているかを1ルールずつ確認していますが、これはLaravel側の責務なので、正常系1本 + 代表的な異常系1本に絞りませんか?
逆に、title が同一ユーザー内で重複していないかのようなこのアプリケーション固有のルールがあれば、そちらのテストを厚くしたいです。

「動くコード」はAIが出してくれるが、
「必要なコードだけになっているか」は人間が判断する必要がある、というのが今のところの実感です。

2. プロジェクトの規約からの逸脱

チーム内ではバリデーションをFormRequest(リクエストの検証ルールを専用クラスに切り出すLaravelの仕組み)に寄せる方針にしているのですが、稀にコントローラ内で Validator を直接使っているコードが上がってきます。

// スレッドの新規登録を受け付けるアクション
// フォームから送られてきた値を検証し、問題なければ保存する
public function store(Request $request)
{
    // 送信された全項目($request->all())を、第2引数のルールで検証する
    // required = 必須, string = 文字列, max:255 = 255文字以内
    //
    // 本来はこの検証部分を FormRequest クラスに切り出す約束になっている
    $validator = Validator::make($request->all(), [
        'title' => ['required', 'string', 'max:255'],
        'body'  => ['required', 'string'],
    ]);

    // 検証に失敗した場合は、エラーメッセージと入力値を持たせて元の画面に戻す
    // withErrors() でエラー内容を、withInput() で入力済みの値を引き継がせている
    if ($validator->fails()) {
        return redirect()->back()->withErrors($validator)->withInput();
    }

    // 検証を通過した場合は、このあとにスレッドの保存処理が続く
    // ...
}

動作としては何の問題もありません。
ただ、プロジェクトとしてFormRequestに寄せている以上、バリデーションの置き場所が2箇所に分かれてしまいます。

前提で書いたとおり、規約を読ませる設定ファイルを用意していないので、こういう出力が混ざるのは仕方のない部分だと思っています。

ただ、この手の指摘には特徴があって、1度指摘すると以降はほぼ再発しません
次のプルリクからはちゃんとFormRequestで出てきます。

ここが、さきほどの過剰なテストとの決定的な違いです。
「規約に合わせる」は覚えれば済む知識なので1回で終わりますが、「このテストは必要か」は毎回判断が要るので、何度でも発生します。

副産物:自分が知らなかった記述に出会える

ここまでは指摘する側の話でしたが、レビュアーである私が学ばされる場面もありました。

一番わかりやすかったのが、アクセサの書き方です。

そもそもアクセサとは

アクセサは、DBから取得したデータをModel側で加工し、DBの値はそのままに、加工後の値をViewへ渡す仕組みです。

たとえば、作成日のカラムに 2026-09-10 10:00:00 というデータが入っているとします。
これをViewでは 2026/09/10 のようにスラッシュ区切りで表示したい。

このとき、View側やコントローラ側で毎回変換するのではなく、「この項目はこう見せる」という定義をModelに1箇所だけ置いておく、というのがアクセサです。

日付のように1つのカラムを加工するほかに、複数のカラムを組み合わせて、DBには存在しない項目を作ることもできます。

私が知っていた書き方

姓と名からフルネームを組み立てる例です。
私はずっとこの書き方で覚えていました。

// first_name と last_name を連結した「フルネーム」を返すアクセサ
// DBに full_name というカラムは存在しないが、$user->full_name で参照できるようになる
//
// メソッド名を get〜Attribute にすると、〜の部分(FullName)が属性名として認識され、
// スネークケースに変換された full_name で呼び出せる
public function getFullNameAttribute(): string
{
    return $this->first_name . ' ' . $this->last_name;
}

プルリクに書かれていた書き方

同じフルネームの定義が、こう書かれていました。

use Illuminate\Database\Eloquent\Casts\Attribute;

// 上と同じく、$user->full_name で参照できるフルネームを定義している
//
// メソッド名(fullName)がそのまま属性名になるため、get〜Attribute の命名が不要
// 戻り値の型が Attribute であることが、アクセサだと判定される目印になっている
protected function fullName(): Attribute
{
    return Attribute::make(
        // get: に「読み取り時の処理」を渡す
        // $attributes には、そのレコードのDB上の値が配列で入っている
        // 書き込み時の処理が必要なら、ここに set: を並べて書ける
        get: fn (mixed $value, array $attributes) => $attributes['first_name'] . ' ' . $attributes['last_name'],
    );
}

Illuminate\Database\Eloquent\Casts\Attribute を使う、Laravel 9で入った書き方です。
メソッド名から get / Attribute が消え、読み取りと書き込みの処理を1つのメソッドにまとめられるようになりました。

これをAIに投げていたら、たぶん「問題なし」で通していました。
自分の知識より新しい記述が出てくるというのは、AIレビュー時代の初学者のプルリクならではです。

レビューの流れ

前提として gh コマンドが使え、対象リポジトリの master を clone 済みの状態です。

  1. descriptionを読んで概要を把握する
  2. git checkout master && git pull
  3. gh pr list でPR番号を確認し、gh pr checkout <番号>
  4. Dockerでローカル環境を立ち上げ、descriptionの動作確認欄と突き合わせながら実際に触る
  5. 差分を読み、コメント or マージ

ポイントは4です。
AIに差分だけ渡すレビューだと、ここが丸ごと飛びます。
実際に触ると「テストは通っているがUXが破綻している」みたいな、差分からは見えないものが引っかかります。

AI時代は、descriptionの整備が重要になった

ここまで書いてきた「増えた指摘」には共通点があります。

コードだけ見ても判断できない、ということです。

過剰なテストかどうかは、書いた人が何を守りたかったのかがわからないと判断できない。
見慣れない記述も、意図的に選んだのか、AIが出したものをそのまま貼ったのかで指摘の内容が変わります。

そこで、descriptionを以下のテンプレートに沿って書いてもらうようにしました。
狙いは明確で、AIが出力したコードをある程度理解していないと、レビュー依頼が出せない状態を作ることです。

このテンプレートはあえて細かくしています。
現場ではここまで求められないことも多いですが、雑なフォーマットに慣れるより、細かいものに慣れてから省略できるようになったほうがいいという判断です。

私が今まで関わってきた案件では、ここまで整備されておらず、みんな結構適当に書いていました。
AI時代はここの整備が効いてくると思っています。

PR descriptionテンプレート(全文)
## 概要
いわゆる結論ベースで、このPRはどういったものを開発したのか。
修正のタスクであれば、何を修正したのかを3行程度で。

例)作成したスレッドがログインユーザー以外(他者)も修正できる状態だったので、
ポリシークラスを使用して認可を追加

## 背景
なぜこの機能開発 or 修正を実施したのか。
このタスクを行うことで、UI/UXの向上が見込まれる、処理速度が向上するなど。
この部分はプログラマーが関与していない可能性もあるので、飛ばしても大丈夫です。

例)スレッドとユーザーの認可が整備されていないのは、
アプリとしてクリティカルな問題だったため

## 変更内容
機能ベースとプログラムベースの両方で記載する。

## 動作確認
どういったテストを行ってこのPRを出したか。
レビュー担当者がローカル環境で動作確認をするため、
これを書いておくとレビュー担当者の確認負荷が軽減する。
「こういうテストをしたのか」「このテストしてなくない?」がわかる。

ポイントは、正常系と異常系の両方を記載すること。

### 正常系
- 問題なく動作するパターン

### 異常系
- こういうテストをしたらちゃんとバリデーションが出た、ということを書く

## レビューして欲しい観点
レビューして欲しい観点、つまりこのPRの懸念点を記載する。
思いつかない場合でも「N+1問題の懸念がないか」など汎用的なものがあるので、
一応記載しておくと丁寧。

## 補足、備考など
もしあれば。
例えば、この実装方法とこちらで迷ったが、今回のPRの方法にした、など。

実際に運用してみると、「レビューして欲しい観点」の欄で理解度がはっきり出ます
ここが空欄か「特になし」の場合、AIの出力をそのまま出している可能性が高い。
逆にここが具体的に書けている人は、コードの中身も把握できています。

レビュアーにとって、descriptionはどこを重点的に見るかの入り口になりました。

まとめ

AIを使わずにレビューしてみて、レビュー観点はこう変わりました。

AI以前 現在
主な指摘 N+1、命名、バリデーション漏れ 過剰実装、不要なテスト
判断基準 動くか / 規約に沿っているか 必要か / 意図を説明できるか
見る対象 コードの差分 差分 + descriptionに書かれた意図

AIは「動くコード」を出してくれます。
ただ、そのコードが必要十分かどうかを判断するのは、結局レビュアー側の基礎力です。
フレームワークの責務がどこまでかを知らなければ、さっきの過剰なテストは「テストが充実している」に見えてしまう。

この時代だからこそ基礎力が効いてくる、というのが今回やってみての結論です。

1
1
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
1
1

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?