前回の記事では、肥大化したControllerをServiceに切り出しました。Controllerの責務が明確になり、テストもしやすくなりました。

しかし、まだ問題が残っています。今回は「更新メソッドが取得処理も兼ねている」問題に取り組みます。

問題の再確認

前回Serviceに切り出したコードを見てみましょう。

class LandingPageService
{
    public function update(int $lpId, array $data, int $userId): array
    {
        // 事前チェック
        // ...

        DB::beginTransaction();
        try {
            // LP本体を保存
            $lp = Lp::updateOrCreate(['id' => $lpId], [
                'name' => $data['name'],
                'status' => $data['status'],
                'template' => $data['template'],
            ]);

            // バージョン作成
            $version = $lp->versions()->create([...]);

            // データ整形
            $formattedData = $this->formatData($data);

            // ステップ・CTAを保存
            $this->saveStepsAndCtas($formattedData, $lp);

            // ステップ削除
            // ...

            // CVページの保存または削除
            // ...

            // 更新日時を更新
            $lp->touch();

            // 保存したLPを再取得 ← ここ
            $lp->refresh();

            // バージョンファイル書き出し
            $version->saveVersionFile($formattedData);

            // レスポンスデータの組み立て ← ここ
            $responseData = [
                'lp_id' => $lp->id,
                'ctas' => $lp->stepsAndCtas['ctas']->pluck('id'),
                'steps' => $lp->stepsAndCtas['steps']->map(fn($step) => [
                    'id' => $step['id'],
                    'related_cta_id' => $step['related_cta_id'],
                ]),
            ];

            // ステータス変更時の副作用
            // ...

            DB::commit();
            return $responseData; // ← データを返している
            
        } catch (\Throwable $e) {
            DB::rollBack();
            throw $e;
        }
    }
}

update()メソッドなのに、更新後のデータを取得して返しています。

なぜこうなるのか

この設計には明確な意図があります。

通信回数を減らしたい

フロントエンド → API: LP更新リクエスト
API → フロントエンド: 更新結果 + 最新データ

1回の通信で更新と取得の両方ができます。

フロントエンドが更新後のデータをすぐ表示したい

// 更新して、すぐに最新データを画面に反映
const response = await api.updateLP(lpData);
this.lp = response.data; // 返ってきたデータで画面更新

別途取得APIを呼ぶ必要がありません。

一見、効率的に見えます。しかし、このトレードオフで複雑さが増しているのです。

何が問題か

1. メソッドの責務が2つになっている

public function update(int $lpId, array $data, int $userId): array

メソッド名はupdateなのに、実際には：

更新: LPデータを保存する
取得: 最新のLPデータを返す

という2つの責務を持っています。

「このメソッドは何をするのか？」という問いに、一言で答えられません。

2. トランザクション内で取得処理が走る

DB::beginTransaction();
try {
    // ... 更新処理 ...
    
    // 保存したLPを再取得
    $lp->refresh();
    
    // レスポンスデータの組み立て
    $responseData = [
        'lp_id' => $lp->id,
        'ctas' => $lp->stepsAndCtas['ctas']->pluck('id'),
        'steps' => $lp->stepsAndCtas['steps']->map(fn($step) => [
            'id' => $step['id'],
            'related_cta_id' => $step['related_cta_id'],
        ]),
    ];
    
    DB::commit();
}

トランザクション内で：

refresh()による再取得
stepsAndCtasリレーションの読み込み
レスポンスデータの組み立て

これらの処理が実行されるため、ロック時間が長くなります。

3. テストの複雑化

public function test_LP更新()
{
    $service = new LandingPageService();
    
    $result = $service->update(
        lpId: 1,
        data: ['name' => 'テストLP', 'status' => 'active'],
        userId: 1
    );

    // 更新が成功したか？
    $this->assertDatabaseHas('lps', [
        'id' => 1,
        'name' => 'テストLP',
        'status' => 'active',
    ]);
    
    // 正しいデータが返ってきたか？
    $this->assertEquals(1, $result['lp_id']);
    $this->assertArrayHasKey('ctas', $result);
    $this->assertArrayHasKey('steps', $result);
    $this->assertCount(3, $result['steps']); // ステップ数は正しいか？
    // ... さらに詳細な検証が必要
}

「更新成功」と「正しいデータが返る」の両方を検証する必要があります。

4. 意図が不明瞭

呼び出し側が、このメソッドから何が返ってくるのか分かりにくい。

// 更新だけしたい場合でも、データが返ってくる
$result = $this->service->update($lpId, $data, $userId);
// $resultは使わないけど、受け取らざるを得ない

逆に、データが必要な場合も：

// 返ってくるデータの構造を知っている必要がある
$result = $this->service->update($lpId, $data, $userId);
$ctaIds = $result['ctas']; // 'ctas'キーがあることを知っている前提

暗黙の知識が必要です。

分離する

更新と取得を分離してみましょう。

更新メソッド（更新だけに専念）

class LandingPageService
{
    public function update(int $lpId, array $data, int $userId): int
    {
        // 事前チェック
        // ...

        DB::beginTransaction();
        try {
            // LP本体を保存
            $lp = Lp::updateOrCreate(['id' => $lpId], [
                'name' => $data['name'],
                'status' => $data['status'],
                'template' => $data['template'],
            ]);

            // バージョン作成
            $version = $lp->versions()->create([...]);

            // データ整形
            $formattedData = $this->formatData($data);

            // ステップ・CTAを保存
            $this->saveStepsAndCtas($formattedData, $lp);

            // ステップ削除
            // ...

            // CVページの保存または削除
            // ...

            // 更新日時を更新
            $lp->touch();

            // バージョンファイル書き出し
            $version->saveVersionFile($formattedData);

            // ステータス変更時の副作用
            // ...

            DB::commit();
            
            // LP IDだけを返す
            return $lp->id;
            
        } catch (\Throwable $e) {
            DB::rollBack();
            throw $e;
        }
    }
}

変更点:

refresh()を削除
レスポンスデータの組み立てを削除
戻り値をLP IDだけに変更

取得メソッド（取得だけに専念）

class LandingPageService
{
    public function get(int $lpId): array
    {
        $lp = Lp::with(['stepsAndCtas'])->findOrFail($lpId);
        
        return [
            'lp_id' => $lp->id,
            'name' => $lp->name,
            'status' => $lp->status,
            'ctas' => $lp->stepsAndCtas['ctas']->map(fn($cta) => [
                'id' => $cta->id,
                'type' => $cta->type,
                'content' => $cta->content,
            ]),
            'steps' => $lp->stepsAndCtas['steps']->map(fn($step) => [
                'id' => $step->id,
                'related_cta_id' => $step->related_cta_id,
                'order' => $step->order,
            ]),
        ];
    }
}

トランザクションも不要で、シンプルです。

Controller

class LandingPageController extends Controller
{
    public function __construct(
        private LandingPageService $service
    ) {}

    public function update(Request $request)
    {
        try {
            // 更新
            $lpId = $this->service->update(
                $request->input('id'),
                $request->all(),
                Auth::id()
            );

            // 最新データを取得
            $lpData = $this->service->get($lpId);

            return $this->success($lpData);
            
        } catch (\Throwable $e) {
            // エラーハンドリング
        }
    }
}

更新後にデータが必要なら、明示的にget()を呼びます。

分離によるメリット

1. 責務が明確になった

// 更新だけする
public function update(int $lpId, array $data, int $userId): int

// 取得だけする
public function get(int $lpId): array

各メソッドが何をするのか、一目瞭然です。

2. トランザクションが軽くなった

DB::beginTransaction();
try {
    // 更新処理だけ
    // refresh()もレスポンス組み立ても不要
    
    DB::commit();
}

ロック時間が短縮され、パフォーマンスが向上します。

3. テストが簡単になった

更新のテスト

public function test_LP更新()
{
    $service = new LandingPageService();
    
    $lpId = $service->update(
        lpId: 1,
        data: ['name' => 'テストLP', 'status' => 'active'],
        userId: 1
    );

    // 更新が成功したかだけを検証
    $this->assertEquals(1, $lpId);
    $this->assertDatabaseHas('lps', [
        'id' => 1,
        'name' => 'テストLP',
        'status' => 'active',
    ]);
}

取得のテスト

public function test_LP取得()
{
    $service = new LandingPageService();
    
    $result = $service->get(lpId: 1);

    // 正しいデータが返るかだけを検証
    $this->assertEquals(1, $result['lp_id']);
    $this->assertArrayHasKey('ctas', $result);
    $this->assertCount(3, $result['steps']);
}

それぞれのテストが独立し、シンプルになりました。

4. 呼び出し側の柔軟性が向上した

更新だけしたい場合

// 更新だけ
$lpId = $this->service->update($lpId, $data, $userId);
// データは不要なので取得しない

更新後のデータが必要な場合

// 更新
$lpId = $this->service->update($lpId, $data, $userId);

// 最新データを取得
$lpData = $this->service->get($lpId);

表示だけ更新したい場合

// 取得だけ
$lpData = $this->service->get($lpId);

必要に応じて組み合わせられます。

5. エラーハンドリングが単純になった

Before（分離前）

try {
    $result = $this->service->update($lpId, $data, $userId);
    // 更新は成功したが、データ組み立て中にエラーが発生したら？
} catch (\Throwable $e) {
    // どの段階で失敗したのか不明瞭
}

After（分離後）

try {
    $lpId = $this->service->update($lpId, $data, $userId);
} catch (\Throwable $e) {
    // 明確に更新処理の失敗
}

try {
    $lpData = $this->service->get($lpId);
} catch (\Throwable $e) {
    // 明確に取得処理の失敗
}

失敗箇所が明確になります。

通信回数は増えるけど

「でも、API呼び出しが2回になってしまう...」

確かに、更新と取得を分離すると：

// Before: 1回
POST /api/lp/update → 更新 + 最新データ

// After: 2回
POST /api/lp/update → 更新
GET /api/lp/:id → 最新データ

通信回数が増えます。

しかし、実際には問題にならないケースが多い

理由1: HTTPは十分に速い

現代のHTTP/2やHTTP/3では、複数のリクエストを効率的に処理できます。数十ms〜数百msの差は、ユーザー体験にほとんど影響しません。

理由2: 必要なときだけ2回呼べばいい

// 更新後、画面遷移する場合（データ不要）
await api.updateLP(lpData);
router.push('/lp/list');

// 更新後、同じ画面で編集を続ける場合（データ必要）
await api.updateLP(lpData);
const latestData = await api.getLP(lpId);
this.lp = latestData;

用途に応じて使い分けられます。

理由3: キャッシュが効く

取得APIはGETなので、適切にキャッシュを設定できます。

public function show(int $lpId)
{
    $lpData = $this->service->get($lpId);
    
    return response()
        ->json($lpData)
        ->header('Cache-Control', 'max-age=60'); // 1分キャッシュ
}

理由4: コードの保守性 > わずかなパフォーマンス差

通信が1回増えることによるパフォーマンスへの影響よりも、コードの保守性が向上することの方が、長期的には価値が高いです。

パフォーマンスが本当に問題なら

もし通信回数削減が本当に必要なら、その時点で最適化すればいい。

// 特定のケースだけ、更新+取得を提供
public function updateAndGet(int $lpId, array $data, int $userId): array
{
    $lpId = $this->update($lpId, $data, $userId);
    return $this->get($lpId);
}

基本は分離しておき、必要に応じて組み合わせたメソッドを追加する方が、柔軟です。

まとめ

更新メソッドが取得処理も兼ねていると：

問題点

メソッドの責務が2つになる
トランザクション内で取得処理が走る
テストが複雑になる
意図が不明瞭

更新と取得を分離することで：

メリット

責務が明確になる
トランザクションが軽くなる
テストが簡単になる
呼び出し側の柔軟性が向上する
エラーハンドリングが単純になる

通信回数は増えますが：

実際にはユーザー体験への影響は小さい
必要なときだけ2回呼べばいい
コードの保守性の方が重要

「通信回数削減」という最適化のために複雑さを抱え込むのではなく、シンプルな設計を保ちながら、必要に応じて組み合わせられる柔軟性を持つことが大切です。

次回の記事では、まだ残っている問題「早期throwによる複雑なフロー」に取り組みます。事前チェックで何度も例外を投げるのではなく、バリデーション結果を集約して一度に返す方法を考えます。