Action Classes

@martinbean for example here:

this is action class

class ImproveFighterSkill
{
    public function execute(Fighter $fighter, int $skillId): array
    {
        DB::beginTransaction();

        try {
            $skill = Skill::findOrFail($skillId);
            $currentSkillLevel = $fighter->skills()->where('skill_id', $skillId)->first()->pivot->level ?? 0;

            if ($currentSkillLevel >= 5) {
                throw new Exception('Skill level cannot exceed the maximum limit of 5.');
            }

            $improvementCost = $skill->price * ($currentSkillLevel + 1);

            if ($fighter->balance < $improvementCost) {
                throw new Exception('Not enough balance to improve skill.');
            }

            $fighter->balance -= $improvementCost;
            $fighter->save();
            $fighter->skills()->syncWithoutDetaching([$skillId => ['level' => $currentSkillLevel + 1]]);

            DB::commit();

            return ['success' => true, 'message' => 'Skill improved successfully.'];
        } catch (Exception $e) {
            DB::rollBack();

            return ['success' => false, 'message' => $e->getMessage()];
        }
    }
}

this is controller:

class ImproveFighterSkillController extends Controller
{
    /**
     * Handle the incoming request.
     */
    public function __invoke(ImproveSkillRequest $request, ImproveFighterSkill $improveFighterSkill): JsonResponse
    {
        $data = $request->validated();
        $user = auth()->user();
        $fighter = $user->fighter;

        $result = $improveFighterSkill->execute($fighter, $data['skill_id']);

        if ($result['success']) {
            return response()->json(['message' => $result['message']], 200);
        }

        return response()->json(['message' => $result['message']], 400);
    }
}

@GeorgeKala With that example, it doesn’t seem there’s anything to “return”. You’re increasing a particular skill for a particular fighter, so the action is either inherently successful, or fails for a particular reason (which you’re already doing by throwing exceptions). However, there are a few improvements that could be made.

• You pass a Fighter instance, but then a primitive value for the skill ID. Why not pass a Skill instance as well?
• You can just use the cleaner closure-based syntax for running queries within a transaction.
• You should create a custom exception class for representing the things that can go wrong, instead of throwing generic Exception instances.

With the above, your action class could be made to look something like this:

class ImproveFighterSkill
{
    public function execute(Fighter $fighter, Skill $skill): void
    {
        DB::transaction(function () use ($fighter, $skill): void {
            $currentLevel = $fighter->skills()->find($skill->getKey())?->pivot?->level ?? 0;

            if ($currentLevel >= 5) {
                throw SkillException::alreadyMaximum();
            }

            $cost = $skill->price * ($currentLevel + 1);

            if ($fighter->balance < $cost) {
                throw SkillException::unaffordable();
            }

            $fighter->decrement('balance', $cost);

            $fighter->skills()->syncWithoutDetaching([
                $skill->getKey() => [
                    'level' => $currentLevel + 1,
                ],
            ]);
        });
    }
}

The action now runs its logic, or throws a SkillException instance if a precondition fails (i.e. fighter has already maximised the given skill, or that they cannot afford the upgrade).

I’d also make your controller accept the skill ID as a route parameter rather than being passed as form data. So having a route that looks something like this:

Route::post('/skills/{skill}/improve', [SkillImprovementController::class, 'store']);

Then you will get the skill injected via route–model binding instead of having to submit it:

public function store(StoreImprovementRequest $request, Skill $skill, ImproveFighterSkill $action): JsonResponse
{
    $fighter = $request->user()->fighter;

    $action->execute($fighter, $skill);

    // Return success response
}

I also only return successful responses from controllers. I use the exception handler for converting domain exceptions (i.e. the SkillException I defined above) to appropriate HTTP responses:

// Convert SkillException instances to 400 Bad Request exceptions
$exceptions->map(SkillException::class, function (SkillException $e) {
    return new BadRequestHttpException($e->getMessage(), previous: $e);
});

This also makes returning success in your response body redundant. Someone will know if the request was “successful” or not by whether they get a 20x response status or not. That’s the entire point of HTTP status codes. You’re never going to throw a 400 Bad Request response with "success": true, are you…?