GitHub Copilotにコードの問題点を洗い出してもらおう！

はじめに

私は、「リファクタリング」（=プログラムの外部から見た動作を変えずにソースコードの内部構造を整理すること）が好きです。ただ、リファクタリングを実際にやろうとするとそこそこ手間がかかります。

1. コードの問題点を洗い出す。
2. 問題のうち、修正する箇所を決め、 修正前と修正後で挙動が変わっていないことを保証するテストを作成する
3. コードを修正する
4. 修正したコードが正しく動作することをテストする
5. 2.〜4.を繰り返す

そこで、私は普段使っているGitHub Copilotにリファクタリングを任せたいので、その第一歩として 「GitHub Copilotにコードの問題点を洗い出してもらうこと」を実験してみました。本記事では、その結果や学びを紹介します。

GitHub Copilotにコードの問題点を指摘してもらう

GitHub Copilotに限らず、AIに何かを依頼する時に大事なことは事前に言葉やルールを定義することです。今回で言えば、GitHub Copilotにコードの問題点を指摘してもらう時に大事なことが、何を問題とするのか？を定義することです。私が依頼したいことをGitHub Copilotに実現してもらうので、GitHub Copilotには私と同じ考えを持ってもらう必要があります。そうでなければ、GitHub Copilotが出力した内容と私が期待する内容が乖離してしまうからです。
今回、私は .github/instructions/coding.instructions.md にコーディングルールを定義することで問題点の洗い出しを試みました(下記)。

---
applyTo: "**/*.php"
---

# コーディングルール

このプロジェクトでは、PHPのファイルに対して以下のコーディングルールを徹底してください。

- クラスは単一責任の原則に従い、1クラス1責務を徹底する。
    - 単一責任の原則とは、クラスは変更される理由が1つだけであるべきという原則です。
- 常にクラスはイミュータブルであるべきなので、setterを持つクラスはなるべく使用しないようにする。クラスに必要な値はコンストラクタで受け取り、プロパティはprivateにする。
    - 例えば、Userクラスがある場合、そのクラスのプロパティ（例えばnameやemailなど）はprivateにし、コンストラクタで初期化します。プロパティを変更するためのsetterメソッドは提供しません。
- プリミティブな値とその値を使用するロジックは、常にクラスにまとめる。値オブジェクトの使用を積極的に検討する。
    - 値オブジェクトは、エンティティとは異なり、識別子を持たず、その属性によってのみ識別されるオブジェクトです。例えば、Emailアドレスや金額などが値オブジェクトの例です。
- 継承の使用をできる限り避け、finalクラスを使用する。継承ではなく、コンポジションを使用する。
- 関心の分離を徹底するために副作用が発生する処理を抽象化する。抽象化はabstractではなく、interfaceを使用する。
    - 副作用とは、関数や処理が「引数と戻り値」以外の外部環境に依存したり影響を与えることです。
    - ただし、このプロジェクトではDBへのアクセスを抽象化することはしません。
- ネットワークの通信が発生する処理は、その通信の回数が最低限になるようにする。
    - 例えば、DBへのアクセスが1回で済むような取得処理をループ内で複数回実行することは避ける。
- メソッドの引数および戻り値には、型ヒントを必ず使用する。
    - 型は可能な限り、プリミティブな型ではなく、クラスを使用する。

## コードの問題の大小に対しての考え方

あなたは、このプロジェクトのソースコードの問題の大きさを下記の順序で考えています。1が最も大きな問題です。 「コードの問題点を洗い出してください」と同じ意味を持つ指示をされた場合、対象コードのどの部分がどのような点で問題か？を提供します。

1. クラスの責務が複数あること
2. ミュータブルなクラスが存在すること
3. 適切に抽象化が使用されていないこと

GitHub Copilotでは、.github/instructions配下に上記のようにルールを定義することが可能です。詳しくは、公式ドキュメントに記載がありますので、こちらを参考にしてください。

実際にやってみた

下記が今回の検証対象のコードです。

namespace App\Console\Commands;

use Illuminate\Console\Command;

class RegisterTodo extends Command
{
    /**
     * The name and signature of the console command.
     *
     * @var string
     */
    protected $signature = 'app:register-todo {title} {description?} {due_date}';

    /**
     * The console command description.
     *
     * @var string
     */
    protected $description = 'Command description';

    /**
     * Execute the console command.
     */
    public function handle()
    {
        $title = $this->argument('title');
        $description = $this->argument('description') ?? '';
        $dueDate = $this->argument('due_date');

        // 禁止ワードチェック
        $ngWords = ['バグ', '失敗'];
        foreach ($ngWords as $word) {
            if (strpos($title, $word) !== false) {
                $this->error("タイトルに禁止ワード '{$word}' が含まれています。");
                return 1;
            }
        }

        // タイトルバリデーション
        if (empty($title)) {
            $this->error('タイトルは必須です。');
            return 1;
        }
        if (strlen($title) > 255) {
            $this->error('タイトルは255文字以内で入力してください。');
            return 1;
        }

        // 説明バリデーション
        if (strlen($description) > 1000) {
            $this->error('説明は1000文字以内で入力してください。');
            return 1;
        }

        // 締切日バリデーション（プリミティブ型で処理）
        if (!preg_match('/^\\d{4}-\\d{2}-\\d{2}$/', $dueDate)) {
            $this->error('締切日はYYYY-MM-DD形式で入力してください。');
            return 1;
        }
        $today = date('Y-m-d');
        if ($dueDate < $today) {
            $this->error('締切日は今日以降の日付を指定してください。');
            return 1;
        }
      
        $apiUrl = 'https://todo.api.com'; //架空のAPIエンドポイント
        $data = json_encode([
            'title' => $title,
            'description' => $description,
            'due_date' => $dueDate,
        ]);
        
        $curl = curl_init($apiUrl);
        curl_setopt($curl, CURLOPT_RETURNTRANSFER, true);
        curl_setopt($curl, CURLOPT_POST, true);
        curl_setopt($curl, CURLOPT_POSTFIELDS, $data);
        curl_setopt($curl, CURLOPT_HTTPHEADER, [
            'Content-Type: application/json',
        ]);
        
        $response = curl_exec($curl);
        $statusCode = curl_getinfo($curl, CURLINFO_HTTP_CODE);
        curl_close($curl);
        
        if ($statusCode >= 200 && $statusCode < 300) {
            $this->info('APIにTodoを登録しました。');
            return 0;
        } else {
            $this->error('APIへの登録に失敗しました。ステータスコード: ' . $statusCode);
            return 1;
        }
    }
}

GitHub Copilotが使用するモデルを「Claude Sonnet3.7」に設定し上記のコードに対して、「コードの問題点を教えてください」とプロンプトで指示をしたところ、下記のような回答が返ってきました。

1. クラスの責務が複数ある
・コマンド引数の取得、バリデーション、APIリクエスト送信と複数の責務が混在しています
・単一責任の原則に違反しています
2. 抽象化が不十分
・APIとの通信処理が直接実装され、抽象化されていません
・バリデーションロジックが直接記述されています
3. プリミティブ型の多用
・タイトル、説明、締切日などがプリミティブ型のまま使用されており、値オブジェクトになっていません
4. 依存関係の注入がない
・外部サービス（API）への依存が直接コード内にハードコードされています
5. 命令型プログラミングの多用
・手続き的なコードが多く、オブジェクト指向の恩恵を受けられていません
6. バリデーションとビジネスロジックの混在
・入力検証とビジネスロジックが同じメソッド内に混在しています
7. エラーハンドリングが不十分
・cURLエラーの詳細な処理が不足しています
8. 指定された責務との不一致
・指示によれば、このクラスはDTOを作成してUsecaseクラスに渡す責務を持つべきですが、実際にはそうなっていません

ご覧の通り、GitHub Copilotは.github/instructions/coding.instructions.mdで定義したコーディングルールに違反した箇所を問題点として洗い出してくれました。素晴らしいです。試しに「値オブジェクトを導入できそうな箇所を洗い出して、私に値オブジェクトのコードを共有してください」と指示をしたら、下記の回答(一部を抜粋)が返ってきました。

<?php

namespace App\ValueObjects;

final class TodoTitle
{
    private string $value;
    private array $ngWords = ['バグ', '失敗'];

    public function __construct(string $value)
    {
        $this->validateNotEmpty($value);
        $this->validateLength($value);
        $this->validateNoNgWords($value);
        $this->value = $value;
    }

    private function validateNotEmpty(string $value): void
    {
        if (empty($value)) {
            throw new \InvalidArgumentException('タイトルは必須です。');
        }
    }

    private function validateLength(string $value): void
    {
        if (strlen($value) > 255) {
            throw new \InvalidArgumentException('タイトルは255文字以内で入力してください。');
        }
    }

    private function validateNoNgWords(string $value): void
    {
        foreach ($this->ngWords as $word) {
            if (strpos($value, $word) !== false) {
                throw new \InvalidArgumentException("タイトルに禁止ワード '{$word}' が含まれています。");
            }
        }
    }

    public function getValue(): string
    {
        return $this->value;
    }
}

正直なところ、バリデーションルールを1つずつメソッドにしているところは少し気になりますが、とてもいいですね！
そして、次に注目して欲しい点がGitHub Copilotが指摘した問題点の8番目の部分です。

8. 指定された責務との不一致
・指示によれば、このクラスはDTOを作成してUsecaseクラスに渡す責務を持つべきですが、実際にはそうなっていません

なぜこのような出力をしてくれたのか？というと、私が.github/instructions/commands.instructions.mdで

---
applyTo: "app/Console/Commands/*.php"
---

# 対象クラスの責務

対象クラスの責務は下記の通りです。「対象クラスの責務は何ですか」と同じ意味を持つ質問をされた場合、下記の内容を提供します。

- 対象のクラスは、CLIコマンドの実行時に呼ばれるクラスです。
- 対象のクラスの責務は、DTOを作成して`app/Usecases`配下にあるUsecaseクラスにDTOを渡すことです。
    - DTOとは、Data Transfer Objectの略で、データ転送オブジェクトのことです。DTOは、複数のデータを一つのオブジェクトにまとめて転送するためのオブジェクトです。DTOは、通常、ビジネスロジックを持たず、単にデータを保持するためのものです。

上記のように定義をしていたためです。GitHub Copilotでは特定のファイルのみに適用したいルールを定義できるので、定義してみました。検証対象のコードのパスは、app/Console/Commands/RegisterTodo.phpなので、GitHub Copilotが上記のルールを認識し、対象クラスの責務と実際のコードが一致していないことを指摘してくれました。これを活用すれば、アーキテクチャに違反しているコードも洗い出せそうですね！

まとめ

• GitHub Copilotに私と同じ考えでコードの問題点を洗い出してもらうことができた。
• GitHub Copilotにコードの問題点を洗い出してもらうためには、事前にルールや言葉の定義をすることが重要である。

先述した検証対象のコードは、業務コードと比較した時に全然複雑ではないですが、GitHub Copilotは業務コードの場合でも私と同じ考えを持ち問題点を指摘してくれたので、とても満足しています。 私には「GitHub Copilotにリファクタリングをしてもらい、その間にティータイムを楽しむ」という夢があるので、夢に向かって研究を続けたいと思います。