3
\$\begingroup\$

I've written an eCommerce shopping cart handler class in PHP. Please review the code and inform me of any errors and/or badly written code and/or badly handled logics. Please also inform me of any missing functionality or security loopholes.

class Cart extends Sessions{

protected $cartProducts = array();
protected $dbLink;
protected $cartName;

public function __construct(){

    parent::__construct(); // to run the constructor of parent
    $this->cartName = 'cart';
    try{
        $this->dbLink = new db;
    }
    catch( Exception $e ){
        echo $e->getMessage() , '\n';
    }
}

public function __destruct(){
    $this->dbLink = FALSE;
}

public function cart_exist(){
    return $this->session_exist($this->cartName);
}

public function create_cart(){
    $this->create_session($this->cartName, true);
    $this->cartProducts = &$_SESSION[$this->cartName];
}

public function add_product( $data ){
    if( !$this->cart_exist() ){
        $this->create_cart();
    }
    $this->insert_value($this->cartName, $data );
}

public function display_cart(){
    $this->display_session($this->cartName);
}

public function del_from_cart( $index ){
    unset($_SESSION[$this->cartName][$index]);
}

public function update_cart_item( $index , array $data ){
    if( array_key_exists( $index , $_SESSION[$this->cartName]) ){
        $arr = &$_SESSION[$this->cartName][$index];
        foreach( $data as $key => $value ){
            $arr[$key] = $data[$key];
        }
    }
}

public function remove_cart(){
    $this->remove_session($this->cartName);
}

}
\$\endgroup\$
  • \$\begingroup\$ It would probably be helpful to link to the Sessions class. For anyone interested, the code is here. \$\endgroup\$ – tim Aug 29 '14 at 18:55
  • \$\begingroup\$ This is the session class, from which this cart class has been inherited. codereview.stackexchange.com/questions/61066/… \$\endgroup\$ – Syntax Error Aug 29 '14 at 19:19
4
\$\begingroup\$

There are a few things:

  • Comments like this one:

        parent::__construct(); // to run the constructor of parent
    

    provide no real value; they just repeat what the code does. Prefer reserving comments for tricky bits of code that aren't self-explanatory.

  • You never use $dbLink; get rid of it. If subclasses need a database, let them create one.

  • __destruct is largely unnecessary here. You gain nothing from detaching references at this point; PHP will already release them once your destructor returns. And if you followed the previous point, the only thing you're releasing no longer exists anyway.

  • All your variables are protected, which implies that you are planning for subclasses to exist. I'm not sure specializing a cart even makes much sense, though.

    Unless you already have subclasses, and those subclasses need basically unrestricted access to those properties, you should make them private. (And even if you do, you might want to reexamine whether inheritance is the proper relationship.)

  • You have a public constructor and a "create_cart" function? That's going to confuse people. (You already created a Cart when you said new Cart.) It looks like what you need here is a name that more clearly describes what the function does.

  • I'm not a big fan of instance method names that include the containing class's name. It feels like a symptom (or maybe a cause) of drifting away from OO concepts and back to proceduralism.

    This is just personal preference; take it as you will. But IMO, if you have a Cart, then from now until it dies, it will always be a Cart. You don't have to keep reminding it of that. :)

  • I see that $cartProducts is a reference to an array in $_SESSION. At that point, both names refer to the exact same value. Any change you make to $this->cartProducts also happens to $_SESSION[$this->cartName], and vice versa.

    Which makes me wonder: Why do so many methods mess with $_SESSION[$this->cartName] rather than with $this->cartProducts?

  • Variable names like $data aren't very descriptive. You know a product is being added; most carts would include a quantity as well. But i don't know for sure what $data looks like, for two big reasons. One is the name. The other...?

  • You let the caller stuff whatever garbage it wants to in your cart.

    A cart keeps track of items (or products), and typically quantities as well, and usually has a way to get a total price. This cart is basically a list of blobs, and can't currently do any of that stuff easily, because it doesn't exert any type of ownership.

    The list of items is your domain, not the caller's. Unless this is intended to be a generic list of "whatever", you decide the structure of what goes into it -- and in return, you get the ability to do stuff over and above what a stock list can do. Otherwise, the caller is actually worse off using your cart than mucking around with the $_SESSION['cart'] array directly. (At least when they're using $_SESSION, they know they're farting around with global state. Your class hides that fact behind what looks like a per-instance interface.)

    You don't necessarily need to say "everything you insert must be an instance of this particular class". You could conceivably let the caller decide that much. But you can -- and probably should -- require that they exhibit certain behaviors (like having a get_price() method). These requirements should be documented. And if you're tracking quantities, then do so explicitly -- consider making $quantity a discrete parameter to the add_product method.

    Once you have decided how an item must behave, then you can do stuff that a typical cart does out of the box. If you decide items must have a price, for example, then now you can add a way to get the total price of all items. That in itself is part of the bare minimum functionality i expect of any shopping cart class. If it doesn't have that, it's just another list. :P

But the really big thing i'm seeing here is the relationship between carts and sessions. A Cart might have a session, or use a session. But it is really a different thing. Inheriting from Sessions seems to me incorrect, unless the parent class is badly named.

It feels to me, though, like you have the relationship the wrong way around. A Cart is obviously part of a session, as evidenced by your bunch of functions to manage subsets of session data.

And PHP lets you put objects in a session.

The cart itself could be session data, at which point $session->gimme('some cart name') can return a whole Cart object, and all this session-management stuff becomes obsolete.

In order to make it work:

  • The class needs to be loaded, or autoloadable, before session_start().

  • The class must be serializable...which requires that anything you need to persist must also be serializable, and/or that anything that's not serializable not be included in the list of properties to store. That's usually not a big deal; the only things PHP can't serialize are resources and quasi-resources like your $dbLink. (One of a number of reasons $dbLink probably isn't worth keeping. But even if you do, you can add a __wakeup() function to reinitialize it.)

The main thing to keep in mind is that if you only want the item info serialized, or you have unserializable fields (like $dbLink), you should provide a __sleep method that tells PHP which properties to serialize. You'll probably also want a __wakeup() method to reinitialize those items that you didn't serialize.

With all that taken care of...what's left?

class Cart {
    private $items = [];

    public function add_product($product, $quantity) {
        $this->items[] = ['product' => $item, 'quantity' => $quantity];
    }

    public function delete_product_at($index) {
        unset($this->items[$index]);
    }

    public function update_product_at($index, $product, $quantity) {
        if (isset($this->items[$index])) {
            $this->items[$index] = ['product' => $product, 'quantity' => $quantity];
        }
    }

    public function get_total() {
        return array_reduce($this->items, function($sum, $item) {
            return $sum + $item['product']->get_price() * $item['quantity'];
        }, 0);
    }



    # don't really need __sleep or __wakeup, since $dbLink is gone.  But it might be
    # helpful to illustrate their use.
    private $some_value;

    public function __sleep() {
        # in `__sleep`, you return a list of properties you want saved
        return ['items'];
    }

    public function __wakeup() {
        # since $some_value wasn't serialized, it doesn't have a value.
        # before you use it again, set it to something sensible.

        $this->some_value = 42;
    }
}

The main missing piece left at this point is your display_cart function. Not sure what it does...but frankly, if its name is accurate, it shouldn't be here. :P

This is a model class, or a business object if you don't care about MVC and such. It's responsible for managing the data, and for representing it in a way that the UI can do something with it. But most schools of thought on software architecture would say this class shouldn't be involved in the actual presentation/rendering. That's the job of the code that manages the UI.

If you want to be able to list the cart's contents, two simple ways would be to have a function that returns $this->items, or implement the IteratorAggregate interface and have a getIterator() function return an iterator over $this->items.

\$\endgroup\$
  • \$\begingroup\$ Woah thank you for such explanatory feedback dude. It will take me some time to read, understand and implement your points. I'll post back for further questions etc soon :) \$\endgroup\$ – Syntax Error Aug 30 '14 at 11:11
  • \$\begingroup\$ I've implemented most of your points. Now about the $data in add_product() method. It is actually an array (which i forgot to mention that time). It contains product_id, quantity. \$\endgroup\$ – Syntax Error Sep 1 '14 at 9:02
  • \$\begingroup\$ yes you are right display_cart() doesn't belong here. \$\endgroup\$ – Syntax Error Sep 1 '14 at 9:03
  • 1
    \$\begingroup\$ @OMI: Well, to the caller, all that really matters is what products, and how many of each, right? So try to hide the rest. Don't even hint at indexes, arrays, etc -- in the item-management API, all that exists are products and quantities. (For instance, instead of an index, use the product/ID as your lookup key.) Then the caller basically can't worry about that stuff. \$\endgroup\$ – cHao Sep 2 '14 at 19:52
  • 1
    \$\begingroup\$ @OMI: Even better, if you have the "add" and "remove" methods update the quantity of an existing item whenever possible (rather than adding or deleting an entry each time), then a Cart is basically an abstraction for "a set of product counts". I'd still like to see a way to get the total price, but that'd be out of scope unless/until you accept whole products rather than just IDs. \$\endgroup\$ – cHao Sep 2 '14 at 20:06
2
\$\begingroup\$

Just a couple of points:

  • Your Cart class violates the SRP.

    You are extending from your session handler class, which you shouldn't because it shouldn't be the cart's responsibility to also handle session management. Your class is having 3 different responsibilities. I would instead separate those two classes and inject Sessions as a dependency into Cart.

  • You have hard-coded the cart name ($this->cartName = 'cart';). Now what if you would want to change the name of the cart? You would have to modify the class to change it, and then still every cart would have the same name. I suggest you make it dynamic by injecting it (into the constructor) so you can define this when instantiating the class. (see dependency injection at the bottom of this answer)

  • display_cart() doesn't belong there, the 3rd responsibility of this class.

    After you've separated the classes they should both have one responsibility. But that would still not be the case because this method prints a HTML formatted string. I would rather rename it to something like getProductsData() which would probably have to return an array with all the products the cart contains. And then you can move the responsibility to print the data in a HTML format to somewhere else.

  • Your class dependency dbLink is redundant, because you aren't using it anywhere. Even if you would use it, I would give the responsibility of persisting anything to a new class. With that being said, the __destruct() method can be deleted.

  • I see you have $this->cartProducts = &$_SESSION[$this->cartName];. Is there any reason why you are assigning the reference of the superglobal to this class property? I fail to see why you would do that, and you aren't using it anywhere. You might as well just return $_SESSION[$this->cartName];.

    Secondly, I'm assuming $cartProducts is supposed to keep track of the products the cart contains, but I don't see your add_product() method adding anything to it. :/

There are a lot of things wrong with this class, I hope this is of any help.

Dependency Injection

public function __construct($name, Sessions $sessions)
{
    $this->cartName = $name;
    $this->sessions = $sessions;
}
\$\endgroup\$
  • \$\begingroup\$ I like the way you used dependency injection. To be honest, initially I went this way, initiating session class and storing its reference in local variable. But I don't remember now why I erased this and chose the other way around i.e. inheriting from session class directly. I will look into your feedback in detail and post back for extended help ;) P.S. Thanks for your support. You guys are awesome! \$\endgroup\$ – Syntax Error Aug 30 '14 at 11:22

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.