I think there is more to discuss here then business logic inside your controller. I'm going to generally keep it to a high level of discussion, but feel free to comment and ask for specifics. I'm going to break it down like this:
- Security issues
- Architecture issues
- Business logic issues
- Formatting issues
Here we go!
Security
These two lines right here are actually quite dangerous:
$productId = $_POST['productId'];
$quantity = $_POST['quantity'];
It doesn't look like you are doing any validation/filtering/cleaning of user input. Both of these variables are copied straight out of the user input and then sent off to various methods (getProductDetailsByProductId
, computeProductPrice
) that have no expectation that they are now dealing with untrusted user input. As a result, the possibility of security vulnerabilities is high. At the very least, you should:
$productId = (int) $_POST['productId'];
$quantity = (int) $_POST['quantity'];
Understanding that user input is untrustworthy, and treating it as such, is a key part of application security. I would very carefully check the rest of your code and make sure that you are vetting all user input everywhere. Are you familiar with the dangers of XSS vulnerabilities and SQL injection? If you're not, now is the time to start learning.
I similarly don't see anything related to CSRF protection, which is a necessity for endpoints such as this. Some frameworks would automatically handle CSRF protection in middleware, but given the overall details of what I see here, I'm suspicious that CSRF protection is simply not happening.
Architecture
The "fatness" of your controller has to do with more than just business logic. You are also instantiating your dependencies directly inside your controller. This makes automated testing impossible, but also opens you up to any number of subtle bugs. Dependency injection is the best way to fix these issues, but if that is too much of an architectural change for you, you could always go with the old school fallback: singletons. It's far from ideal, but it is easier to implement than dependency injection, and will at least make it so that you can't accidentally open up multiple connections to the database in response to a single HTTP request.
To be clear, these are the lines I'm talking about:
$connection = new Connection();
$session = new SessionModel();
Although to be fair, all of your object instantiation is a symptom of the problem. What it really comes down to is a lack of proper separation of concerns. Your controller is in charge of everything: not just business logic, but also application logic. This makes it substantially harder to manage your application in the long-run.
Business Logic
Getting to your question, I definitely think that you have too much business logic in your controller. The issue is that you are looking at this from the perspective of "I've got this one place where I add items to the cart". It never works that way. An e-commerce system naturally starts that way, but you will find your needs will quickly grow over time. How do you handle the situation when someone says: "Hey, can I look at my order history and click a button to add an old order back into my cart?". That is a super common feature for an e-commerce platform. Unfortunately, adding an item to a user's cart currently involves multiple dependencies and 50ish lines of code. If you ever need to add to a cart, in any other way, and at any other endpoint, you will very quickly realize the pain caused by fat controllers like this.
So what should it look like? This is what your controller method should look like:
public function processAddingAndUpdatingOfCartItems(cart $cart)
{
// Get values from request
$productId = (int) $_POST['productId'];
$quantity = (int) $_POST['quantity'];
if ($quantity <= 0)
return 'Invalid quantity';
if (!$cart->is_valid_product($productId))
return 'Invalid product';
if (!$cart->add_product($productId, $quantity))
return 'There was an error processing your request';
return redirect('/view_cart');
}
All of this business logic should be handled by the cart
. You should not instantiate the database connection: in fact, your controller shouldn't even need it. You preferably shouldn't even create the cart: it should be injected in. Worst-case scenario though, the cart is the only thing you should have to create.
The cart should handle validation (which it can defer to a cartValidator, if desired). The cart should defer storage of itself to the session object, and the cart shouldn't care about whether the session exists or not. That is the session
objects job. The session
object should simply be told "store this", and the session
object should automatically create the actual session, if need be. Ideally this would all happen via dependency injection.
These are just a couple points, but it all boils down to the same thing: to get anything done, your controller has to know about everything. It would help to read up on the single responsibility principle. The more things your code has to "know" about to get its job done, the more room there is for things to break in the future, and the harder it is to update your application.
Keep in mind that the goal here isn't just to shuffle everything into a cart
class. Rather, the cart
class itself defers most of its responsibility to its own child classes: cartValidator
, cartProduct
, session
, etc... A lot of the stuff you have right now needs to be better divided into some of these classes, so that each class does only the things that are directly related to its own functionality, and defers the rest to someone else. However, you should have one cart
object that acts as a single entry point for all cart-related functionality: adding products, verifying product information, etc...
Formatting
This one isn't too bad, but the way you line up your variables is a little wonky. Many code bases avoid lining things up all together, but if you would rather do that in your code base, obviously that's perfectly reasonable. I think though that if you are going to do this you should find some consistent stop-points for lining stuff up at. If you put some of your lines of code right next to eachother, I think you'll see the problem:
$productTable = new ProductTable($connection);
$productDetails = $productTable->getProductDetailsByProductId($productId);
$productModel = new Product();
$productPrice = $productModel->computeProductPrice($productId, $quantity);
$cartItem = new CartItemModel();
$cartValidator = new CartValidator();
$doesCartExistForCurrentUser = $cartValidator->doesCartExistForCurrentUser();
$cartTable = new CartTable($connection);
$cartItemTable = new CartItemTable();
The inconsistency can be distracting, and the last thing you want while looking at code is to be distracted. Code gets read much more than it is written, so it is best to write it consistently.
Again, I'm happy to delve into more details on any particular item, if you are interested.
Dependency Injection
Dependency injection is not a small thing, and won't be a small change for your system architecture. That being said, dependency injection will be an important change for your architecture, and, if done right, will substantially improve your organization and long-term maintainability. As Sam mentioned, learning about SOLID is a good place to start. The idea though is that you don't directly instantiate your dependencies. Rather, they are given to you. This is typically done via type-hinting in your constructor. In many ways, your other classes are already doing this. The trick therefore is to implement a dependency injection container. Again, this isn't necessarily a small thing, so do some reading first, find an open-source one instead of building your own, and start small. It is absolutely worth the effort.
In practice it works because your dependency injection container builds all of your objects for you, and uses reflection to determine the dependency each object needs to do its job. So your controller would look like this:
class cart_controller{
public function __construct(cart $cart, request $request){
$this->cart = $cart;
$this->request = $request; // stores details of the HTTP request
}
}
Your cart then might look like this:
class cart{
public function __construct(session $session, cartItemFactory $cartItemFactory, cartValidator $cartValidator){
// store dependencies
}
}
Your application bootstrapping process builds the controller with the dependency injection container, the dependency injection container sees that the controller needs the cart, it sees the cart needs the session, itemFactory, and validator, and then it will find any necessary dependencies of those things as well. It starts at the bottom of the dependency tree, building the things that have no dependencies, passing those into the next objects and building them, and all the way up until you have a controller that gets a cart object when it is instantiated.
It can be complicated, which is why it is best to just use a pre-built DI container. It is also a completely different way of building your application, which is why it is best to spend some time learning and start small. This isn't the sort of thing you just "do" over a weekend. The biggest advantage though is that it allows you to actually test your code. Having maintained large applications both with and without automated testing, I can tell you that in the long run having some automated tests (unit tests, integration tests, etc...) in place is a huge boon to your ability to maintain your code-base, and introduce new features quickly. That alone is absolutely worth the time it will take you to implement these large changes.
Then again, some of your classes are already receiving some of their dependencies through their constructor arguments, so you're also kind of halfway there.
Responding to your questions
Regarding whether or not to have the DB connection passed in to the controller constructor: As a general rule of thumb, if you have a controller with methods that have completely different dependencies, then they should probably be in different controllers. It all comes down to the Single Responsibility Principle and proper separation-of-concerns, which you will read plenty about if you start learning about SOLID. As another option though, it is common in some PHP MVC frameworks to do method-level dependency injection, where each controller-method can have additional dependencies injected in. I'm not really a fan of it myself (partly for the above reasons), but it is a common pattern, so you wouldn't be crazy to do it.
Regarding my comment about how all your object instantiation is a symptom of the problem, what I mean is that directly creating objects violates the D in SOLID, which Sam mentioned in his answer. When you directly create your dependencies, rather than allowing them to be given to you, you tightly couple a class to its sub-classes, which makes your application more rigid, harder to maintain, and impossible to test. Again, this are large architectural changes, so it is probably best to start by reading up on these topics, and practicing them with smaller applications before doing anything crazy with your code base.
refreshCartTotalAmount
a method inherited from a parent class or trait, or is that defined elsewhere? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Nov 30 '17 at 23:55