Fat-free MVC Controller

Question

I have this class containing a method that performs a few processes for displaying the payment page.

class Payment extends Controller
{
    public function displayPaymentPage()
    {
        $session   = new Session();

        if(is_null($session->getSessionKey('userId'))) {
            header('Location: /login');
        } else {
            $cart = new Cart();

            if($cart->doesCartExistForCurrentUser()) {
                $session        = new Session();
                $cartId         = $session->getSessionKey('cartId');

                $connection     = new PDOConnection();
                $cartTable      = new CartTable($connection);
                $cartItemTable  = new CartItemTable($connection);

                $data['cartAmountDetails']  = $cartTable->getCartTotals($cartId);
                $data['cartItems']          = $cartItemTable->getCartItemsByCartId($cartId);

                $this->viewPresenter->display('basic', '/payment/payment', $data, 'Payment');
            } else {
                header('Location: /cart');
            }
        }
    }
}

And this other class for the Shipping entity:

class Shipping extends Controller
{
    public function displayShippingForm()
    {
        $session = new Session();

        if(is_null($session->getSessionKey('userId'))) {
            // Set flag so user will be redirected to the Shipping page after loggin in
            $session->setSessionKey('cartPage', true);
            header('Location: /login');
        } else {
            $cart = new Cart();
            $data = $this->getShippingInformation($cart, $session);
            $data['errors']['shipping'] = $session->getFlashFormErrors('shipping');

            $this->viewPresenter->display('basic', 'shipping/shipping', $data, 'Shipping');
        }
    }

    // Contents of this were originally entirely inside the above method
    // Decided to refactor it
    // Wasn't sure if it goes into a Service class instead,
    //        since it depends on different entities
    private function getShippingInformation(Cart $cart, Session $session)
    {
        if($cart->doesCartExistForCurrentUser()) {
            $cartId         = $session->getSessionKey('cartId');

            $connection     = new PDOConnection();
            $cartTable      = new CartTable($connection);
            $totalWeight    = $cartTable->getCartTotals($cartId)->total_weight;

            $shippingTable  = new ShippingTable($connection);
            $shippingModel  = new ShippingModel($shippingTable);

            $data['groundShipping']      = $shippingModel->getGroundShippingParticulars($totalWeight);
            $data['expeditedShipping']   = $shippingModel->getExpeditedShippingParticulars($totalWeight);

           return $data;
        } else {
            header('Location: /cart');
        }
    }
}

And I have this one service class:

class CartItemService
{
    // The contents of this function were originally inside the controller
    // But I realized they may be considered one unit of work
    // So I decided to group them together
    // Is putting them inside the Service class an overkill
    // or should I have just refactored inside the Controller?
    public function saveCartItem($cartId, CartItem $cartItem, CartItemTable $cartItemTable, Product $product, ProductTable $productTable)
    {
        $productDetails         = $productTable->getProductDetailsByProductId($cartItem->productId);
        $productPrice           = $product->computeProductPrice($productTable, $cartItem->productId, $cartItem->quantity);
        $cartItem->weight       = $productDetails->weight;
        $cartItem->unitPrice    = $productDetails->price;
        $cartItem->price        = $productPrice;
        $cartItem->cartId       = $cartId;

        $cartItemTable->insertCartItem($cartItem);
    }
}

My other question is, aside from those in the comments above, can any of these be considered business logic? I've always been pretty unsure about the scope of business logic.

For example, testing the uniqueness of email addresses, checking if the user is logged in, checking if an entity already exists before proceeding -- are those considered business logic? And whether those checks should be further encapsulated inside their corresponding Models, or if it's okay to perform simple null checks in the controller. What are some examples of logical checks that are allowable inside the Controller?

Answer 1

Business logic or not

I guess the general dichotomy is View logic v/s Business logic. So, to decide whether something is part of your application business logic, try to imagine your app having a completely different UI, like a command-line UI. Would the logic still need to part of your application with this new UI? If yes, then it's likely your business logic.

Email validation and checking if user is logged in, seem to me as part of business logic.

Controllers

The role of a Controller is the most vague one in MVC world. That's why there exist several Model-View-* patterns: Model-View-Presenter, Model-View-ViewModel, Model-View-Adapter.

To answer your question:

What are some examples of logical checks that are allowable inside the Controller?

Better to first think what is the responsibility of a particular controller and what's the suitable level of abstraction for the code that accomplishes that responsibility?

For example in both controllers you have the following checks:

if(is_null($session->getSessionKey('userId'))) {

This code is bothering us with the minutiae of how user data is stored within the session. These details shouldn't be our concern while deciding what to display on Payments page. All we really want to ask is:

if(!$user->isLoggedIn()) {

Both controllers instantiate a Session object. (Confusingly inside Payment Controller two Session objects are created, I suspect that the creation of the second one is not actually needed.) Seeing new Session() within controller code I'm thinking that it starts a new session, but really it's just using the current session. The controller needs session to do its work. If a class or function needs something, it should ask for it. Best way to ask for something is through a parameter.

Similarly the controllers create PDOConnection objects. Is it really the job of a controller to set up database connection? Could the connection instead be provided for the controllers.

Services

What is a service? Saying that something is a service, doesn't really tell us much about what it does. Like the name CartItemService only tells us that it does something with CartItems. We could have equally well have called it CartItemHelper or CartItemUtil. Its real responsibility is storing CartItems to database. So why not instead call it CartItemStorage.