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株式会社エフカフェ

【どう実装する？シリーズ #6】更新メソッドがデータを返す、それって本当に必要？

Posted at

前回の記事では、肥大化した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による複雑なフロー」に取り組みます。事前チェックで何度も例外を投げるのではなく、バリデーション結果を集約して一度に返す方法を考えます。

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?