Go to Qiita Advent Calendar Top
LoginSignup
0
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

@sho_fcafein株式会社エフカフェ

【どう実装する？シリーズ #5】肥大化したController、Serviceに切り出せば十分？

Posted at

前回の記事では、traitでの共通化からドメインオブジェクトへのリファクタリングを振り返りました。

今回は、LP（ランディングページ）保存処理という複雑な更新処理が抱える問題と、その改善について考えます。この処理は複数の問題を抱えており、今回はまず「Controllerの肥大化」に焦点を当て、Serviceへの切り出しを検討します。

LP保存処理の現状

現在のLP保存処理は、Controllerに直接実装されています。まずはコードを見てみましょう。

class LandingPageController extends Controller
{
    public function update(Request $request)
    {
        // 早期return 1: 新規作成時の上限チェック
        $lp = Lp::find($request->id);
        if (!$lp && !Auth::user()->canCreateLP()) {
            return $this->error('LP作成上限に達しています');
        }

        // 早期return 2: ABテスト関連の制約チェック
        if ($lp && $lp->hasActiveABTest()) {
            if ($this->hasNoVisibleSteps($request->steps)) {
                return $this->error('表示ステップが必要です');
            }
            if ($request->status === 'inactive') {
                return $this->error('公開中のABテストがあります');
            }
        }

        // 早期return 3: 公開時のメディア存在チェック
        if ($request->status === 'active') {
            $mediaIds = $this->extractMediaIds($request);
            if (!$this->allMediaExists($mediaIds)) {
                return $this->error('メディアが見つかりません');
            }
        }

        $oldStatus = $lp?->status;

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

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

            // リクエストデータを内部形式に整形
            $formattedData = $this->formatData($request->all());

            // ステップ・CTAを保存(メディア存在チェックも内包)
            $this->saveStepsAndCtas($formattedData, $lp);

            // 暗黙的な削除対象の検出とマージ
            $currentStepIds = $this->collectStepIds($formattedData);
            $orphanedStepIds = $lp->steps()
                ->whereNotIn('id', $currentStepIds)
                ->pluck('id')
                ->toArray();
            $allDeleteIds = array_merge(
                $request->input('delete_steps', []),
                $orphanedStepIds
            );

            // ステップ削除(関連CTAも連鎖削除)
            if (!empty($allDeleteIds)) {
                $relatedCtaIds = $lp->steps()
                    ->whereIn('id', $allDeleteIds)
                    ->pluck('related_cta_id')
                    ->toArray();
                $lp->steps()
                    ->whereIn('id', array_merge($allDeleteIds, $relatedCtaIds))
                    ->delete();
            }

            // CVページの保存または削除
            if (empty($request->cv_page)) {
                CvPage::where('lp_id', $lp->id)->delete();
            } else {
                CvPage::updateOrCreate(['lp_id' => $lp->id], ['value' => $request->cv_page]);
            }

            // 更新日時を更新
            $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'],
                ]),
            ];

            // ステータス変更時の副作用: フォールバックJSON
            if ($oldStatus !== $lp->status) {
                if ($lp->status === 'active') {
                    Storage::put("fallback/{$lp->id}.json", json_encode($formattedData));
                } else {
                    Storage::delete("fallback/{$lp->id}.json");
                }
            }

            DB::commit();
            return $this->success($responseData);
            
        } catch (\Throwable $e) {
            DB::rollBack();
            throw $e;
        }
    }
}

約100行にわたる巨大なメソッドです。ぱっと見ただけでも、複雑で長いことが分かります。

問題点の洗い出し

このコードには、いくつもの問題が含まれています。

1. Controllerにビジネスロジックが直書き

HTTPリクエストを受け取るControllerに、ビジネスロジックがすべて書かれています。

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

// 保存した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'],
    ]),
];

更新処理なのに、最後に再取得してレスポンスデータを組み立てています。

3. 早期returnが多くフローが複雑

if (!$lp && !Auth::user()->canCreateLP()) {
    return $this->error('LP作成上限に達しています');
}

if ($lp && $lp->hasActiveABTest()) {
    if ($this->hasNoVisibleSteps($request->steps)) {
        return $this->error('表示ステップが必要です');
    }
    // ...
}

if ($request->status === 'active') {
    // ...
}

処理の途中で何度もreturnするため、フローが追いにくくなっています。

4. データ整形ロジックが混在

// リクエストデータを内部形式に整形
$formattedData = $this->formatData($request->all());

HTTPリクエストのデータ構造と、内部で扱うデータ構造が異なり、その変換処理も含まれています。

5. 暗黙的な副作用

// 暗黙的な削除対象の検出とマージ
$currentStepIds = $this->collectStepIds($formattedData);
$orphanedStepIds = $lp->steps()
    ->whereNotIn('id', $currentStepIds)
    ->pluck('id')
    ->toArray();
$allDeleteIds = array_merge(
    $request->input('delete_steps', []),
    $orphanedStepIds
);

リクエストで明示的に削除指定されていないステップも、自動的に削除されます。この挙動はコードを読まないと分かりません。

6. トランザクション内で重い処理

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

トランザクション内でファイル書き込みなどの重い処理を実行しているため、ロック時間が長くなります。

これらの問題を一度にすべて解決するのは大変です。まずは「Controllerの肥大化」に焦点を当てて改善していきましょう。

まずはServiceに切り出す

Controllerに直接書かれたビジネスロジックを、Serviceクラスに切り出します。

Serviceクラスの作成

class LandingPageService
{
    public function update(int $lpId, array $data, int $userId): array
    {
        // 事前チェック
        $lp = Lp::find($lpId);
        if (!$lp && !$this->canCreateLP($userId)) {
            throw new LpLimitException('LP作成上限に達しています');
        }

        if ($lp && $lp->hasActiveABTest()) {
            if ($this->hasNoVisibleSteps($data['steps'])) {
                throw new ValidationException('表示ステップが必要です');
            }
            if ($data['status'] === 'inactive') {
                throw new ValidationException('公開中のABテストがあります');
            }
        }

        if ($data['status'] === 'active') {
            $mediaIds = $this->extractMediaIds($data);
            if (!$this->allMediaExists($mediaIds)) {
                throw new MediaNotFoundException('メディアが見つかりません');
            }
        }

        $oldStatus = $lp?->status;

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

            // バージョン作成
            $version = $lp->versions()->create([
                'name' => 'バージョン' . ($lp->versions()->count() + 1),
                'user_id' => $userId,
            ]);

            // リクエストデータを内部形式に整形
            $formattedData = $this->formatData($data);

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

            // 暗黙的な削除対象の検出とマージ
            $currentStepIds = $this->collectStepIds($formattedData);
            $orphanedStepIds = $lp->steps()
                ->whereNotIn('id', $currentStepIds)
                ->pluck('id')
                ->toArray();
            $allDeleteIds = array_merge(
                $data['delete_steps'] ?? [],
                $orphanedStepIds
            );

            // ステップ削除
            if (!empty($allDeleteIds)) {
                $relatedCtaIds = $lp->steps()
                    ->whereIn('id', $allDeleteIds)
                    ->pluck('related_cta_id')
                    ->toArray();
                $lp->steps()
                    ->whereIn('id', array_merge($allDeleteIds, $relatedCtaIds))
                    ->delete();
            }

            // CVページの保存または削除
            if (empty($data['cv_page'])) {
                CvPage::where('lp_id', $lp->id)->delete();
            } else {
                CvPage::updateOrCreate(['lp_id' => $lp->id], ['value' => $data['cv_page']]);
            }

            // 更新日時を更新
            $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'],
                ]),
            ];

            // ステータス変更時の副作用: フォールバックJSON
            if ($oldStatus !== $lp->status) {
                if ($lp->status === 'active') {
                    Storage::put("fallback/{$lp->id}.json", json_encode($formattedData));
                } else {
                    Storage::delete("fallback/{$lp->id}.json");
                }
            }

            DB::commit();
            return $responseData;
            
        } catch (\Throwable $e) {
            DB::rollBack();
            throw $e;
        }
    }

    private function canCreateLP(int $userId): bool
    {
        // LP作成可能数のチェックロジック
    }

    private function hasNoVisibleSteps(array $steps): bool
    {
        // 表示ステップの存在チェックロジック
    }

    private function extractMediaIds(array $data): array
    {
        // メディアID抽出ロジック
    }

    private function allMediaExists(array $mediaIds): bool
    {
        // メディア存在確認ロジック
    }

    private function formatData(array $data): array
    {
        // データ整形ロジック
    }

    private function saveStepsAndCtas(array $formattedData, Lp $lp): void
    {
        // ステップ・CTA保存ロジック
    }

    private function collectStepIds(array $formattedData): array
    {
        // ステップID収集ロジック
    }
}

Controller（シンプルに）

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

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

            return $this->success($responseData);
            
        } catch (LpLimitException $e) {
            return $this->error($e->getMessage(), 400);
        } catch (ValidationException $e) {
            return $this->error($e->getMessage(), 400);
        } catch (MediaNotFoundException $e) {
            return $this->error($e->getMessage(), 404);
        } catch (\Throwable $e) {
            Log::error('LP保存エラー', ['exception' => $e]);
            return $this->error('システムエラーが発生しました', 500);
        }
    }
}

Serviceに切り出すメリット

1. Controllerの責務が明確になった

Before（Controller直書き）

  • HTTPリクエストのハンドリング
  • バリデーション
  • ビジネスロジック実行
  • トランザクション管理
  • エラーハンドリング
  • レスポンス生成

After（Service切り出し後）

  • HTTPリクエストのハンドリング
  • Serviceの呼び出し
  • 例外のハンドリング
  • レスポンス生成

Controllerは「HTTPの入口」としての責務に集中できるようになりました。

2. テストがしやすくなった

Before（Controller直書き）

public function test_LP保存()
{
    // HTTPリクエストを組み立てる必要がある
    $response = $this->postJson('/api/lp/update', [
        'id' => 1,
        'name' => 'テストLP',
        // ...大量のパラメータ
    ]);

    $response->assertStatus(200);
}

HTTPレイヤーを通す必要があり、テストの準備が大変でした。

After（Service切り出し後）

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

    $this->assertEquals(1, $result['lp_id']);
}

Serviceを直接呼び出せるため、純粋にビジネスロジックのテストができます。

3. 再利用性が向上した

// 別のコンテキストから呼び出せる
class LpImportJob implements ShouldQueue
{
    public function __construct(
        private LandingPageService $service
    ) {}

    public function handle()
    {
        foreach ($this->importData as $data) {
            $this->service->update(
                lpId: null, // 新規作成
                data: $data,
                userId: $this->userId
            );
        }
    }
}

Job、コマンド、他のServiceなど、どこからでもLP保存処理を再利用できます。

4. 依存関係が明確になった

public function __construct(
    private LandingPageService $service
) {}

Controllerが依存しているのはLandingPageServiceだけです。依存関係がコンストラクタで明示されています。

こうしておけばよかった：まだ残る問題

Serviceに切り出しただけでは、まだ問題が残っています。

問題1: 更新メソッドが取得処理も兼ねている

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

// レスポンスデータの組み立て
$responseData = [
    'lp_id' => $lp->id,
    'ctas' => $lp->stepsAndCtas['ctas']->pluck('id'),
    // ...
];

更新メソッドなのに、更新後のデータを取得して返しています。これにより：

  • メソッドの責務が「更新」と「取得」の2つになっている
  • トランザクション内で取得処理が走る
  • テストで「更新成功」と「正しいデータが返る」の両方を検証する必要

更新と取得は分離すべきです。次回の記事では、この問題に取り組みます。

問題2: 早期returnならぬ早期throw

if (!$lp && !$this->canCreateLP($userId)) {
    throw new LpLimitException('LP作成上限に達しています');
}

if ($lp && $lp->hasActiveABTest()) {
    if ($this->hasNoVisibleSteps($data['steps'])) {
        throw new ValidationException('表示ステップが必要です');
    }
    // ...
}

例外による早期脱出が多く、フローが複雑です。

問題3: 暗黙的な副作用が残っている

// 暗黙的な削除対象の検出とマージ
$orphanedStepIds = $lp->steps()
    ->whereNotIn('id', $currentStepIds)
    ->pluck('id')
    ->toArray();
$allDeleteIds = array_merge(
    $data['delete_steps'] ?? [],
    $orphanedStepIds
);

削除対象の自動検出は、コードを読まないと分からない挙動です。

これらの問題は、今後の記事で一つずつ改善していきます。

まとめ

肥大化したControllerをServiceに切り出すことで：

得られたメリット

  • Controllerの責務が明確になった
  • テストがしやすくなった
  • 再利用性が向上した
  • 依存関係が明確になった

しかし、まだ残る問題

  • 更新メソッドが取得処理も兼ねている
  • 早期throwによる複雑なフロー
  • 暗黙的な副作用

Serviceに切り出しただけでは不十分で、さらなる改善が必要です。

次回の記事では、「更新メソッドが取得処理も兼ねている」問題に焦点を当て、更新と取得を分離する方法を考えます。更新処理は更新だけに専念し、取得が必要なら明示的に取得処理を呼ぶ設計について議論します。

通信回数を減らすための「最適化」が、実はコードを複雑にしているかもしれません。

0
0
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
Sign upLogin
0
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?