Firstly it would be a great idea to put all of your RESTful (store, update, show, index, destroy/delete) methods within a Service class, and name them, by convention, store, update, show, index, or destroy/delete, so they can be used by your controllers.
I suggest reading up on RESTful Web Services (this is very important)...Your Controller classes aren't RESTful, and don't follow Laravel conventions since the naming of your Service class methods and Controller methods don't follow REST or Laravel conventions.
Inside the BoPromotionController, the getPromotions() method should be in a Service class called PromotionService, and instead of "getPromotions" it should be simply "index()", the createPromotion() method should be "store()", and the deletePromotion() should be simply "delete()", etc... This adheres to best practices and follows REST.
And all of the business logic within the BoPromotionController should be inside the PromotionService class and inject the PromotionService class within the BoPromotionController methods the way you did here
public function createPromotion(CreatePromotionRequest $request, PromotionService $promotionService
the createPromotion method name should be "store".
the getPromotion method should look like this
public function index(PromotionService $promotionService){
$promotion = $promotionService->index();
etc...
}
the deletePromotion method should look like this
public function delete(PromotionService $promotionService, string $id){
$promotionService->delete($id);
You may keep private methods within the Service classes if you need to for handling the business logic.
When you define the RESTful methods within the Service classes you may inject them within a Controller method such as the store method like so
public function store(CreatePromotionRequest $request, PromotionService $promotionService)
{
$validatedData = $request->validated();
try {
$promotion = $promotionService->store($validatedData);
return response()->json([
'message' => 'Promotion and rewards created successfully',
'promotion' => $promotion->load('rewards') // load rewards relationship
]);
} catch (Exception $e) {
return response()->json([
'error' => 'Failed to create promotion',
'details' => $e->getMessage(),
], 500); //internal server error
}
}
Note how I used the method "store" to keep up the Laravel conventions and at the same time follow REST, instead of using createPromotion(...).
Also I noticed within BoPromotionController class you have used "find()" in the deletePromotion() and changeStatus() methods and then create a response with a 404 not found status code, this can be simplified for better readability and to keep your code more clean as so
From this ->
$promotion = Promotion::find($id);
if (!$promotion) {
return response([
'error' => 'Promotion Not Found!',
], 404);
}
To this ->
$promotion = Promotion::findOrFail($id);
findOrFail will automatically throw a 404 not found when you use it, simplifying the code.
The same thing happens in the BoUserController class inside the getTransactions() method. Use findOrFail(); simply.
If you correct the code from up above, and when you use a tool like Postman or whatever tool you use, type in an id that does not exist in the database for the model and the findOrFail() method will throw a 404 not found page, so you can see for yourself.
Something similar happens in the PlayerPromotionController class. In the claimPromotion method you have
$promotion = Promotion::where('code', $validatedData['promotion_code'])->first();
if(! $promotion) {
return response(['message' => 'Promotion not found',], 404);
}
this can be simplified to just
$promotion = Promotion::where('code', $validatedData['promotion_code'])->firstOrFail();
again firstOrFail will return a 404 not found.
Another thing is to look into "return type declarations" in Laravel. I think you might already know about return type declarations since you use them in your PromotionService class but inside your Controller methods you don't. For example, in the BoPromotionController you aren't using the JsonResponse return type declaration, it should look like this for each Controller method
public function createPromotion(CreatePromotionRequest $request, PromotionService $promotionService): JsonResponse {...}
public function deletePromotion(CreatePromotionRequest $request, PromotionService $promotionService): JsonResponse {...}
Make sure to import use Illuminate\Http\JsonResponse;
etc...
In the PromotionService class, inside the claim() method, you are throwing a RuntimeException, not sure why... While the RuntimeException works, a custom exception provides much clearer and more specific context about why the claim failed, which is beneficial for code maintainability, debugging, and handling the error in the calling code. Use a custom exception like
throw new PromotionAlreadyClaimedException('This promotion has already been claimed');
Lastly, I noticed that you aren't using a 201 status code for the createPromotion() method for the BoPromotionContrller, for some reason when you do in other Controllers
try {
$promotion = $promotionService->create($validatedData);
return response()->json([
'message' => 'Promotion and rewards created successfully',
'promotion' => $promotion->load('rewards') // load rewards relationship
], 201);
} catch (Exception $e) {
return response()->json([
'error' => 'Failed to create promotion',
'details' => $e->getMessage(),
], 500); //internal server error
}
Also get into the habit of creating Unit Tests, this is very important as well.
I suggest reading on RESTful web services, and also brush up on how to use Service classes and the Single Responsibility Principal from SOLID. And also sign up and register on Laracasts.com, it will help a lot.
