0

(Disclaimer : I'm asking this question here because as it is opinion-based, it have been closed from https://stackoverflow.com/questions/64050308/an-action-in-my-controller-has-too-much-responsibility)

Summarize the problem

This question has come to me because I write a function to set a current cart for a user.

Specifications :

A user can have multiple shopping cart open. He can grab a few items for one cart and then set it aside to fill another. As soon as he wishes, he can return to any current cart to fill it again or validate it.

Some code

So I have been writing the following Controller :

#src/Controller/CartController
class CartController extends AbstractController
{

/**
* Add product to given cart. If $cart_id equal to 0, create a new cart.
*/
public function addProduct(int $cart_id, int $product_id, Request $request, EntityManagerInterface $entityManager) : RedirectResponse {...}

/**
* Set given cart as the current one for the user
*/
public function setCurrentCartForUser(int $cart_id, Request $request) : RedirectResponse {...}
...
}

This is fine, but now I want to use the function setCurrentCartForUser() into addProduct() like this :

public function addProduct(int $cart_id, int $product_id, Request $request, EntityManagerInterface $entityManager) : RedirectResponse {
   // ... Add product to cart ...
   if($request->query->get('assign_current_cart') === true) $this->setCurrentCartForUser($cart_id, $request)
   return new RedirectResponse($request->headers->get('referer'));
}

Problem

I wondering if that's the better way to do it, because now addProduct() have a lot more responsability than just adding product to cart :

It create instance of new shopping cart if none given ($cart_id === 0) It add product to cart And it also will set the current cart if you give the assign_current_cart option. I was thinking about solving that by moving some responsability to a new action like that :

public function addProductAndSetCurrentCart(int $cart_id, int $product_id, Request $request, EntityManagerInterface $entityManager, CartService $cartService){
   $this->addProduct(int $cart_id, int $product_id, Request $request, EntityManagerInterface $entityManager, CartService $cartService);
   $this->setCurrentCartForUser($cart_id, $request);
   return new RedirectResponse($request->headers->get('referer'));
}

But this seams not ok. I'm looking for the best way to proceed. I was just trying to do it the right way so I can progress, but having my question closed from stackoverflow makes me think that maybe I shouldn't be asking myself so many questions. If this one got close too, I'll find the best way by myself, but I hope that I can learn from this community. Please point out my mistakes, it really help me.

I was thinking about :

  • Creating a new layer on top of the Controller, but that's not MVC anymore I guess
  • I was thinking to create a new Service... But it'll more act as a library.

What I really would like to have would be to make addProduct() only adding product, and setCurrentCartForUser() only setting current cart. Then I'll call them together when I need to for a maximum of flexibility. But as a user can call only one action at a time, the best approach so far seams to be this one :

public function addProduct(int $cart_id, int $product_id, Request $request, EntityManagerInterface $entityManager) : RedirectResponse {
   // ... Add product to cart ...
   if($request->query->get('assign_current_cart') === true) $this->setCurrentCartForUser($cart_id, $request)
   return new RedirectResponse($request->headers->get('referer'));
}

EDIT

Use case :

  • a) User add product to cart from product list page but don't already have one. Product is added to new cart. The new cart become the current one.
  • b) User add product to cart from product list page and already have a current cart. So the product is added to the current cart. No current cart is set.
  • c) User add product to cart from cart list page, product is add to cart. No current cart is set.

I made a little UML (even though I don't master UML)

enter image description here

  • Is it a requirement that a user can add a product to an arbitrary basket and optionally make that the current basket in a single action? Or can the system be made such that a) a product can only be added to the current basket. Making a existing/new basket current is a separate action, or b) adding a product to a basket always makes that the current basket? – Bart van Ingen Schenau Sep 25 at 10:24
  • @BartvanIngenSchenau I have add a use case list to answer your comment ! Hope it'll help you to understand better. – vincent PHILIPPE Sep 25 at 11:01
  • But well, the more I think about it, the more I thinking that to make thing simple I'll maybe only allow user to add product to the current cart... It help me to write all of this – vincent PHILIPPE Sep 25 at 11:13
2

You are correct that your addProduct function is starting to do too much.

Based on the use cases you mentioned, I would split this slightly differently than you had done:

  1. addProduct only adds a product to an existing cart. No change in which cart is active.

  2. addProductToNewCart is a new Controller that coordinates its actions over other controllers.

    1. It invokes the createEmptyCart controller to create a new cart
    2. It invokes the selectCurrentCart controller to set the newly created cart as the current cart
    3. It invokes the addProduct controller to add the product to the new cart.

    In your UML diagram, this would be a new use case that includes the existing use cases for "Create empty cart", "Select a current cart" and "Add product to cart".

| improve this answer | |
  • You say so I've got to create 4 Controller !? addProductToNewCart, createEmptyCart, selectCurrentCart, addProduct – vincent PHILIPPE Sep 25 at 12:30
  • I think you meant addProductToNewCart (Controller) and its functions as : createEmptyCart, selectCurrentCart, addProduct... Sorry if I missunderstood – vincent PHILIPPE Sep 25 at 12:32
  • @vincentPHILIPPE, yes you would get 4 Controllers for this. But 2 of them (createEmptyCart and selectCurrentCart) would also be needed for the corresponding use cases in your diagram. – Bart van Ingen Schenau Sep 25 at 14:39
  • I think I get the idea ! But when you say invokes, it confuse me... I mean, did you think about dependency injection here ? Because it seams weird to me to call a controller function from another one. Did it not break the MVC pattern ? Isn't it better to have createEmptyCart , selectCurrentCart and addProduct inside a service like ProductService ? But maybe it'll break the single responsibility principle as there is a lot of Responsibility for only one Service – vincent PHILIPPE Sep 25 at 15:16
  • Sorry for being so anoying, I really try to understand thing before coding it. – vincent PHILIPPE Sep 25 at 15:17
2

The logic which adds a product to a cart, sets a cart as the current one and creates a new cart belongs to the model. It does not belong to the controller.

This is the fundamental problem you have, which you wouldn't have in the first place if the responsibilities were better split.

So, think of CartController as the manager in the Cart department. This guy receives an email from the client telling him someone wants to add a product to a cart with a unique identifier of 0, 1, etc. As a manager, his role is to delegate. Once he gets all the information needed for his co-workers to do the job properly, he'll simply send this request to one or more services, which will try to do the job and tell him once it's done. After that, his job is to tell the client everything went X way.

Now let's get more technical and think about the API which calls the controller in the first place.

To me, addProduct and setCurrentCart is a fine API. It's simple, easy to use and easy to understand. Let's keep that.

Now as for the use cases :

a) User add product to cart from product list page but don't already have one. Product is added to new cart. The new cart become the current one.

The request will look like : {cart_id: 0, product_id: 22} The service will recognize 0 as being non-existent, therefore it'll create a new cart, and set it as the current cart for the user. We do not call an action (setCurrentCart) from the controller here, the service knows it has to set this cart as being the current cart for the user, this is hidden in the domain which knows its rules and logic. The controller does not know those rules at all.

b) User add product to cart from product list page and already have a current cart. So the product is added to the current cart. No current cart is set.

The request will look like : {cart_id: 10, product_id: 22} The service will recognize 10 as being a cart which belongs to the user. It'll therefore successfully add the product to this very cart, no new cart is created, no cart is set to current.

c) User add product to cart from cart list page, product is add to cart. No current cart is set.

This is the exact same request as b).

Now as for d), which could be to set a cart to being the current cart, well you guessed it, simply pass in the cart_id, validate it belongs to the user, and set it to the current cart in a service. This call to the service is different from adding a product, and would only do this setCurrentCart task. It'd make sense for this service to be reused by the service adding a product, in case it needs to set a cart as the current one.

| improve this answer | |
  • This would involve that creating a new cart set it as the current one. And that would work. But every time I create a new cart I need to check if no other current cart is already set, and if so, I should reset the current option to false for the previous one. – vincent PHILIPPE Sep 25 at 13:05
  • Sure, as long as the domain handles this logic, you're fine. The controller won't have to change one bit and you can split responsibilities inside the domain as you want. – Steve Chamaillard Sep 25 at 13:09
  • Yes, I'm sorry, what do you mean by the domain ? Are you talking about the Model or something related to Business logic ? – vincent PHILIPPE Sep 25 at 13:18
  • Yes I assume that's what you mean : en.wikipedia.org/wiki/Business_logic Ok so what I'll do is creating a services where I put all the business logic ! CartService will have a getOrCreateCart() function and it'll manage everything about getting or creating a new cart ! I'll even add there a setCurrentCart() function so it can be reuse from anywhere ! Does it look right ? – vincent PHILIPPE Sep 25 at 13:27
  • That's a subjective question. It's all up to you. As long as you think your code is as simple as it could be to read & to write, then it's fine. There's no absolute especially since your code and requirements are subject to change. Just remember to keep objects simple. And yes, to answer your original question, the domain, the model, you can call it however you want, is where the business logic happens. Here's an amazing answer on SO explaining what the model is (and is not) : stackoverflow.com/questions/5863870/… – Steve Chamaillard Sep 25 at 14:08
1

It would be fine to have addProduct create a new cart if you had a way to pass the new cart back to the caller.

Currently it looks like you are relying on some form of global state that the function updates, which is a bit of a code smell.

You could change that global state to some sort of parent object for the carts, say ShoppingSession, then you can pass and return this from the function, adding new carts to ShoppingSession.Carts and adjusting ShoppingSession.CurrentCart as required. But it seems like this would be a bit clumsy and large an object.

I think the overall solution here is to move all the "new cart", "add product" logic to the client side. Then have a single Add/Update cart serverside method which takes a whole cart object with products.

Ask why you need to add products to carts instead of just persisting carts with UpdateCart(cart)

| improve this answer | |
  • Hello, this is required, because A cart doesn't have Product collection, but a ProductCart Collection which is an entity who got three property : Cart, Product, Quantity. This is required because I use Doctrine, and as using a ORM, I have to hink in Object POV instead of query. So that's why I can't 'just' persisting carts... And ShoppingSession.Carts looks a good idea, but I think it's quite overkill IMO (I doesn't provide thos kind of details because it'll add to confusion) – vincent PHILIPPE Sep 25 at 12:38
  • 1
    well you can just save the cart with the productcarts and have the same solution. On the server side you can divide it into create cart (if it doesnt exist) and addproducts (that arent already added) but do it in a single call – Ewan Sep 25 at 12:41
  • 1
    But I don't rely on a global state that the function update. It is updating only the database. Actually I update the user_cart table which have the following propery cart_id, user_id, current – vincent PHILIPPE Sep 25 at 12:43
0

You always need a cart. You seem to have two cases: 1. Use current card, or create new cart and make it current if there is no current card. 2. Use a specific cart, or create a new one if it doesn’t exist.

So you call one of two methods that are guaranteed to return a cart, then pass that cart to the method adding the product to a cart.

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.