patternphpMinor
Shopping Cart Handler Class
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:
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
create one.
-
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
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
(And even if you do, you might want to reexamine whether inheritance is the proper
relationship.)
-
You have a public constructor and a "
people. (You already created a
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
then from now until it dies, it will always be a
reminding it of that. :)
-
I see that
point, both names refer to the exact same value. Any change you make to
Which makes me wonder: Why do so many methods mess with
-
Variable names like
added; most carts would include a quantity as well. But i don't know for sure what
-
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
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
tracking quantities, then do so explicitly -- consider making
parameter to the
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
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
can return a whole
In order to make it work:
-
The class needs to be loaded, or autoloadable, before
-
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
-
Comments like this one:
parent::__construct(); // to run the constructor of parentprovide 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 themcreate one.
-
__destruct is largely unnecessary here. You gain nothing from detaching references atthis 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 toexist. 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 confusepeople. (You already created a
Cart when you said new Cart.) It looks like what youneed 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 keepreminding it of that. :)
-
I see that
$cartProducts is a reference to an array in $_SESSION. At thatpoint, 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 beingadded; 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'retracking quantities, then do so explicitly -- consider making
$quantity a discreteparameter 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 parentclass 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.