Business logic inside MVC Cart Controller method

Question

I have a controller method, invoked when a user adds the product he is viewing to the cart.

The way I understand how MVC works, this can be considered a fat controller already, but I'm still not completely sure.

And if that's the case, then how can I possibly refactor this code?

public function processAddingAndUpdatingOfCartItems()
{
    // Get values from request
    $productId  = $_POST['productId'];
    $quantity   = $_POST['quantity'];

    // Instantiate DB Connection object
    $connection = new Connection();

    // Get product details of currently viewed product to be added to cart
    $productTable   = new ProductTable($connection);
    $productDetails = $productTable->getProductDetailsByProductId($productId);

    // Get price of currently viewed product: desired quantity * unit price
    $productModel = new Product();
    $productPrice = $productModel->computeProductPrice($productId, $quantity);

    // Store properties in an array
    $cartItemProperties = array(
        'productId'     => $productId
        ,'quantity'     => $quantity
        ,'unitPrice'    => $productDetails->price
        ,'price'        => $productPrice
    );

    // Create Cart Item Model
    $cartItem   = new CartItemModel();
    $cartItem->setCartItemProperties($cartItemProperties);

    // Check if a cart already exists for the current session
    $cartValidator                  = new CartValidator();
    $doesCartExistForCurrentUser    = $cartValidator->doesCartExistForCurrentUser();

    // Instantiate CartTable and CartItemTable for database queries
    $cartTable      = new CartTable($connection);
    $cartItemTable  = new CartItemTable();

    // If a cart already exists for the current session
    if($doesCartExistForCurrentUser) {
        // Get cart id in the current session
        $session        = new SessionModel();
        $cartId         = $session->getSessionKey('cartId');

        // Insert item into cart
        $cartItemTable->insertItemIntoCart($cartItem);

        // Refresh total amount of cart
        $this->refreshCartTotalAmount($cartTable, $cartId);
    } else {
        // If no cart exists yet for the current session

        // Set default user id to 0
        $sessionUserId = 0;
        $session        = new SessionModel();

        // Check if user is logged in
        $customerValidator = new CustomerValidator();
        if($customerValidator->isUserLoggedIn())
        {
            $sessionUserId  = $session->getSessionKey('userId');
        }

        // Create new cart model containing necessary properties
        $cart = new CartModel();
        $cartProperties = array(
            'customerId' => $sessionUserId
            // other properties
        );

        // Insert into carts table
        $last_cart_id = $cartTable->insertCart($cart);

        // Set cartId property of Cart Item Model
        $cartItem->cartId = $last_cart_id;

        // Insert item into cart
        $cartItemTable->insertItemIntoCart($cartItem);

        // Refresh total amount of cart
        $this->refreshCartTotalAmount($cartTable, $last_cart_id);

        // Set session cart id

        $session->setSessionKey('cartId', $last_cart_id);
    }
}

Answer 1

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:

1. Security issues
2. Architecture issues
3. Business logic issues
4. 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.

Answer 2

I agree with many of the points in Conor Mancone's answer. Sanitation of data is important - one could utilize PHP's Sanitize filters with filter_var(). However, you did mention you are using prepared statements for the queries, so that should be sufficient if you really want to sacrifice security.

And like Conor suggested, try to split the code in that large controller method into separate methods - not only for re-use, but also for unit testing.

There are some duplicate lines of code in your sample - e.g. $session = new SessionModel(); in both cases (ie. when a cart does/doesn't exist for the current session). While it isn't exactly the best example, it partly violates the Don't Repeat Yourself principle.

If you aren't familiar with the S.O.L.I.D. principles then I suggest you look into it- especially D. - Dependency Inversion. That article mentions cases where a model constructor expects a database connection (just like your code).

I have thought about instantiating the DB Connection object in the Controller classes' constructors instead, but I was given the idea that not all methods of a Controller class may need it.¹

What I often do is create a static method in a Database class (or a helper function if it doesn't make sense to put it with the controller) to get an instance of a connection when needed. This is known as the Singleton Pattern.

Consider the case where there is a Database wrapper class - e.g. your Connection class:

class Connection {
    private static $_connectionInstance;
    private static function getConnection() {
        if (!isset(self::$_connectionInstance)) {
            self::$_connectionInstance = new Connection();
        }
        return self::$_connectionInstance;
    }

}

Then use Connection::getConnection() instead of $connection in whatever methods need that.

---

¹_{Comment on Conor's answer}