HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Shopping Cart Handler Class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
handlercartclassshopping

Problem

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);
}

}

Solution

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 sto

Code Snippets

parent::__construct(); // to run the constructor of parent
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;
    }
}

Context

StackExchange Code Review Q#61467, answer score: 4

Revisions (0)

No revisions yet.