Be part of JetBrains PHPverse 2026 on June 9 – a free online event bringing PHP devs worldwide together.

Quadrubo's avatar

Seperate Business Logic with dependant data

Hello, I'm currently struggling with the concept of keeping my business logic seperate from my controller. I extracted the function addToShoppingList into my Model (for now, could be service).

public function addToShoppingList(ShoppingList $shoppingList, string $shoppingGroup): int
    {
        $existingItems = $shoppingList
            ->shoppingListItems()
            ->where('shopping_group', $shoppingGroup)
            ->pluck('category_id')
            ->values();

        $count = 0;

        $this->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;
    }
public function batchStore(BatchStoreShoppingListItemRequest $request)
    {
        $shoppingList = ShoppingList::find($request->input('shoppingList'));
        $recipe = Recipe::find($request->input('recipe'));
        $shoppingGroup = $request->input('shoppingGroup');

        $count = $recipe->addToShoppingList($shoppingList, $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',
            );
        }
    }

My problem is, that this has to return the $count because the Controller needs it, to display the flash messages depending on that. How would I usually extract this away? Would you put flash messages in another part completely? Putting them into the model too seems out of place.

0 likes
7 replies
LaryAI's avatar
Level 58

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:

  1. Create a ShoppingListService class that contains the addToShoppingList method.
  2. Inject this service into your controller.
  3. 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.

Quadrubo's avatar

@LaryAI This doesn't answer my question regarding returning $count from the Service. This still feels wrong to me. What If I depend on more data, return an array? Is there any way to clean this up better, extract this away somehow?

I know Larry is a bot and won't answer, just putting this out for others :)

Snapey's avatar

Don;t see whats wrong with your approach apart from the fact that you are heading towards giving the model far too much responsibility.

I would never do this unless this function is going to be called from multiple places in your code.

I would either create a shoppingListService class or create smaller action classes with one function

2 likes
Quadrubo's avatar

@Snapey Okay I extracted this to a service. Still returning $count from the addToShoppingList function feels wrong to me. What If I need to display messages inside my controller based on more variables, do I then return an array? And If I use that function in a different part of my code that doesn't need flash messages, the count is returned anyways. It feels like I could seperate this better to overcome this problem but I don't know how. Thanks for your help :)

Snapey's avatar
Snapey
Best Answer
Level 122

@Quadrubo You could investigate DTO's then you update the DTO and pass that back and forth. The shopping list DTO would then create the count(s). I say counts because you want to differentiate between number of items and number of new items.

Yes, your messages should come from the controller because this is user presentation information not business logic.

1 like
Greynix's avatar

@Snapey please can we chat in private , I really need help on something urgent … on my laravel project

Please or to participate in this conversation.