It's a good practice to keep your controllers slim and your business logic separated, typically in a service layer. The addToShoppingList method does seem to contain business logic, so it's a good candidate to be moved into a service class. The service class can then be responsible for handling the logic and returning the necessary information back to the controller.
Regarding the flash messages, they are a part of the presentation layer and should be kept in the controller or a dedicated presentation layer class, not in the model or service layer. The service should only return data that the controller can use to decide which flash message to set.
Here's how you might refactor the code:
- Create a
ShoppingListServiceclass that contains theaddToShoppingListmethod. - Inject this service into your controller.
- Call the service method from the controller and use the returned data to set the appropriate flash message.
Here's an example of what the service class might look like:
class ShoppingListService
{
public function addToShoppingList(ShoppingList $shoppingList, Recipe $recipe, string $shoppingGroup): int
{
$existingItems = $shoppingList
->shoppingListItems()
->where('shopping_group', $shoppingGroup)
->pluck('category_id')
->values();
$count = 0;
$recipe->categories->each(function ($category) use ($shoppingList, $shoppingGroup, $existingItems, &$count) {
if (!$existingItems->contains($category->id)) {
ShoppingListItem::create([
'amount' => $category->pivot->amount,
'shopping_group' => $shoppingGroup,
'category_id' => $category->id,
'shopping_list_id' => $shoppingList->id,
]);
$count++;
}
});
return $count;
}
}
And here's how you might refactor the controller:
public function batchStore(BatchStoreShoppingListItemRequest $request, ShoppingListService $shoppingListService)
{
$shoppingList = ShoppingList::find($request->input('shoppingList'));
$recipe = Recipe::find($request->input('recipe'));
$shoppingGroup = $request->input('shoppingGroup');
$count = $shoppingListService->addToShoppingList($shoppingList, $recipe, $shoppingGroup);
if ($count === 0) {
flash(
severity: 'warn',
summary: $count . ' items added to your shopping list',
detail: 'Items are already on your shopping list, try choosing another group'
);
} else if ($count !== $recipe->categories->count()) {
flash(
severity: 'success',
summary: $count . ' items added to your shopping list',
detail: 'Some items are already on your shopping list'
);
} else {
flash(
severity: 'success',
summary: $count . ' items added to your shopping list',
);
}
// Redirect or return response
}
In this refactored code, the ShoppingListService is responsible for the business logic, and the controller is responsible for handling the HTTP request/response cycle, including setting flash messages based on the result from the service. This separation of concerns makes your code cleaner, more maintainable, and easier to test.