My perfect shopping cart class

Question

Here is my shopping cart class! I want it to be perfect and never to have to implement it.

What improvements would you do?

New : IComparable

interface IComparable
{
    public function equals($object);
}

New : ICartItem

interface ICartItem extends IComparable
{
    public function setQuantity($quantity);
    public function getQuantity();
}

New : ICart

interface ICart
{
    public function add(ICartItem $item);
    public function remove(ICartItem $item);
    public function getQuantity(ICartItem $item);
    public function setQuantity(ICartItem $item, $quantity);
    public function isEmpty();
    public function getItems();
    public function clear();
}

New : Cart

class SessionCart implements ICart
{
    const IDENTIFIER = '_CART_';
    protected $items;

    public function __construct(&$container = null)
    {
        if (is_null($container)) {
            if (session_id() == '') {
                session_start();
            }
            if (!isset($_SESSION[self::IDENTIFIER])) {
                $_SESSION[self::IDENTIFIER] = array();
            }
            $container = & $_SESSION[self::IDENTIFIER];
        }
        $this->items = & $container;
    }

    public function add(ICartItem $item)
    {
        $index = $this->getIndexOfItem($item);

        if ($index == -1) {
            $this->items[] = $item;

        } else {
            $item = $this->items[$index];
            $item->setQuantity($item->getQuantity() + 1);
        }

        return $item->getQuantity();
    }

    public function remove(ICartItem $item)
    {
        $index = $this->getIndexOfItem($item);

        if ($index == -1) {
            throw new Exception('The item isn\'t inside the cart.');
        }

        $item = $this->items[$index];

        $quantity = $item->getQuantity() - 1;

        if ($quantity > 0) {
            $item->setQuantity($quantity);

        } else {
            unset($this->items[$index]);
        }
        return $quantity;
    }

    public function getQuantity(ICartItem $item)
    {
        $index = $this->getIndexOfItem($item);

        if ($index == -1) {
            return 0;

        } else {
            return $this->items[$index]->getQuantity();
        }
    }

    public function setQuantity(ICartItem $item, $quantity)
    {
        if (($quantity = (int)$quantity) < 1) {
            throw new Exception('A positive quantity is required.');
        }

        $index = $this->getIndexOfItem($item);

        if ($index == -1) {
            $item->setQuantity($quantity);
            $this->items[] = $item;

        } else {
            $item = $this->items[$index];
            $item->setQuantity($quantity);
        }

        return $item->getQuantity();
    }

    public function isEmpty()
    {
        return empty($this->items);
    }

    public function getItems()
    {
        return $this->items;
    }

    public function clear()
    {
        $this->items = array();
    }

    private function getIndexOfItem(ICartItem $item)
    {
        foreach ($this->items as $key => $value) {
            if ($item->equals($value)) {
                return $key;
            }
        }
        return -1;
    }
}

Answer 1

General

You should reconsider the naming of some methods. For example,

interface IComparable
{
    public function equals($obj);
}

is more clear about what it really does (comments are omitted to save space; of course, proper DocBlock comments should always be included with your code). This way, you can easily add other methods like isLessThan, isGreaterOrEqual, and so on. A method named compareTo as in

$a->compareTo($b)

I'd expect to be a valid callback for usort, thus returning -1 on $a < $b, 0 on $a == $b, or 1 on $a > $b.

Cart

One day, you might want to store your cart items in a database, so using $_SESSION directly is not optimal. I'd define a cart interface:

interface ICart
{
    public function add(IComparable $obj);
    public function remove(IComparable $obj);
    public function getQuantity(IComparable $obj);
    public function setQuantity(IComparable $obj, $qty);
    public function isEmpty();
    public function getAll();
    public function clear();
}

class Cart implements ICart
{
    const IDENTIFIER = '_CART_';
    protected $container;

    public function __construct(&$container = null)
    {
        if (is_null($container)) {
            if (session_id() == '') {
                session_start();
            }
            if (!isset($_SESSION[self::IDENTIFIER])) {
                $_SESSION[self::IDENTIFIER] = array();
            }
            $container = &$_SESSION[self::IDENTIFIER];
        }
        $this->container = &$container;
    }
}

With this approach, you can always build other cart implementations, like a database-aware one. The Cart class can be provided with an array, which will be used to store the data. If you want, you can pass in the $_SESSION superglobal directly, or use any other array. If omitted, a kind of namespace within the session variables (self::IDENTIFIER, '_CART_') is used to store the data.

Since the session - if needed - is started within the constructor, you can rely on its existence in the subsequent methods. BTW: the existence of the $_SESSION array does not guarantee that a session has been started! Use session_id() to check that instead.)

Internal Data Structure

Since your data array can be accessed from outside the cart, the current data structure is prone to get out of sync. It is more robust to swap the indices. Then you can replace getIndex() with getEntry(), which makes the handling much easier.

class Cart implements ICart
{
    ... // see above

    public function add(IComparable $obj)
    {
        $entry = &$this->getEntry($obj);
        $entry['quantity']++;

        return $entry['quantity'];
    }

    public function remove(IComparable $obj)
    {
        $entry = &$this->getEntry($obj);
        $entry['quantity'] = max(0, --$entry['quantity']);

        return $entry['quantity'];
    }

    public function getQuantity(IComparable $obj)
    {
        $entry = &$this->getEntry($obj);
        return $entry['quantity'];
    }

    public function setQuantity(IComparable $obj, $qty)
    {
        $entry = &$this->getEntry($obj);
        if ($entry['quantity'] > 0) {
            $entry['quantity'] = (int) $qty;
        }
        return $entry['quantity'];
    }

    public function isEmpty()
    {
        $total = 0;
        foreach ($this->container as $entry) {
            $total += $entry['quantity'];
        }
        return $total <= 0;
    }

    public function getAll()
    {
        $cart = array();
        foreach ($this->container as $entry) {
            if ($entry['quantity'] > 0) {
                $cart[] = $entry;
            }
        }
        return $cart;
    }

    public function clear()
    {
        $this->container = array();
    }

    private function &getEntry(IComparable $obj)
    {
        foreach ($this->container as &$entry) {
            if ($obj->equals($entry['item'])) {
                return $entry;
            }
        }
        $entry = array(
            'item' => $obj,
            'quantity' => 0
        );
        $this->container[] = &$entry;
        return $entry;
    }
}

As you can see, all methods (excl. isEmpty()) become much simpler.

Answer 2

Based on @nibra's solution

• Same ICompareable interface.

• Same ICart interface but derived from IteratorAggregate.

  interface ICart extends IteratorAggregate {}
  

CartItem class

• Storing an item.
• Storing the quantity if the item.

final class CartItem {

    private $_item;
    private $_quantity;

    public function __construct(IComparable $item, $quantity = 1) {
        $quantity = (int)$quantity;
        if ($quantity < 1) {
            throw new Exception("Invalid quantity");
        }

        $this->_item = $item;
        $this->_quantity = $quantity;
    }

    public function GetQuantity() {
        return $this->_quantity;
    }

    public function GetItem() {
        return $this->_item;
    }
}

Changed internal behavior

• Cart isn't the best name for this kind of implementation because it's not reflecting anything about the implementation, so call it SessionCart.
• session_start(); doesn't belong here because starting the session isn't the responsibility of this class; if we wan't to ensure that the session has been started we need another abstraction above the $_SESSION array.
• $_SESSION as storage backend implies some kind of singleton behavior so we need to handle multiple instance problem with the SessionCart class but this also not the responsibility of SessionCart class because it depends on the framework.
• & operator is bad because it can mess up our code and it really hurts performance; in PHP 5 and above we never really need to use it, see CartItem class.
• quantity: 0; why would we store items with the quantity of 0? If the quantity is 0 then we don't have any item of that type so remove it.

class SessionCart implements ICart {
    protected $container;

    public function __construct($storageIndex = "_CART_") {
        if (isset($_SESSION[$storageIndex])) {
            throw new Exception($storageIndex . " exists in SESSION array");
        }

        $_SESSION[$storageIndex] = $this;
    }

    public function add(IComparable $obj, $quantity) {
        $quantity = (int)$quantity;

        if ($quantity < 1) {
            //throw exception?
            return 0;
        }

        $index = $this->getIndexOfEntry($obj, $quantity);

        if ($index == -1) {
            $this->container[] = new CartItem($obj);
            return 1;
        } 

        $this->container[$index] = new CartItem($obj, $this->container[$index]->GetQuantity() + $quantity);
        return $this->container[$index]->GetQuantity();
    }

    public function remove(IComparable $obj) {
        $index = $this->getIndexOfEntry($obj);
        unset($this->container[$index]);
    }

    public function getQuantity(IComparable $obj) {
        $index = $this->getIndexOfEntry($obj);
        return $index == -1 ? 0 : $this->container[$index]->GetQuantity();
    }

    public function setQuantity(IComparable $obj, $qty) {
        $qty = (int)$qty;

        if ($qty < 1) {
            $this->remove($obj);
            return;
        }

        $index = $this->getIndexOfEntry($obj);

        if ($index == -1) {
            $this->container[] = new CartItem($obj, $qty);
        } else {
            $this->container[$index] = new CartItem($obj, $qty);
        }
    }

    public function isEmpty() {
        return empty($this->container);
    }

    public function getAll() {
        return new SplFixedArray($this->container);
    }

    public function clear() {
        $this->container = array();
    }

    private function getIndexOfEntry(IComparable $obj) {
        foreach ($this->container as $key => $entry) {
            if ($obj->equals($entry->GetItem())) {
                return $key;
            }
        }

        return -1;
    }

    public function getIterator() {
        return new ArrayIterator($this->container);
    }

}

Problem

The code above still depends on $_SESSION array which is bad (hard dependency on a global variable is bad and global variables are always bad) because anyone can modify it's content. I have mentioned another abstraction above the $_SESSION array in the previous section and I think it's a must have to do scenario if we want a perfect implementation of a session based cart.

Answer 3

I would reccomend to use if(session_id() == ''){ //do } instead of if(!isset($_SESSION)) { // do} because $_SESSION can be set while the session is closed.