PHP shopping cart class

Question

The following is a shopping cart class. The initial version of this was pulled from a production application. Since then it has changed a bit but I'd love to hear your feedback on this.

Due to having multiple files, the full source can be found here.

Cart.php

<?php

namespace maldoinc\utils\shopping;

use maldoinc\utils\shopping\persistence\CartPersistentInterface;

class Cart implements \Countable
{
    /* @var $items CartItem[] */
    protected $items = array();

    /** @var CartPersistentInterface */
    protected $intf = null;

    /**
     * @param CartPersistentInterface $intf
     */
    public function __construct(CartPersistentInterface $intf = null)
    {
        $this->intf = $intf;

        if ($this->intf !== null) {
            $this->load();
        }
    }

    /**
     * Save the shopping cart data
     */
    public function save()
    {
        $this->intf->save(serialize($this->items));
    }

    /**
     * Load shopping cart data.
     *
     * Overwrites any existing items the cart might have
     */
    public function load()
    {
        $items = $this->intf->load();

        if ($items !== null) {
            $this->setItems(unserialize($items));
        }
    }

    /**
     * Clears the shopping cart
     */
    public function clear()
    {
        $this->items = array();

        if ($this->intf !== null) {
            $this->intf->clear();
        }
    }

    /**
     * Determines whether the shopping cart is empty or not
     *
     * @return bool
     */
    public function isEmpty()
    {
        return $this->count() === 0;
    }

    /**
     * (PHP 5 &gt;= 5.1.0)<br/>
     * Counts the items of the shopping cart
     * @link http://php.net/manual/en/countable.count.php
     * @return int
     */
    public function count()
    {
        return count($this->items);
    }

    /**
     * Returns the item based on it's identifier
     *
     * @param $identifier
     * @return CartItem
     */
    public function get($identifier)
    {
        return $this->getItemAt($this->indexOf($identifier));
    }

    /**
     * Returns a copy of the shopping cart items
     *
     * @return CartItem[]
     */
    public function getItems()
    {
        return $this->items;
    }

    /**
     * Set the items of the shopping cart
     *
     * @param CartItem[] $items
     */
    public function setItems($items)
    {
        $this->items = $items;
    }

    /**
     * @return float
     */
    public function getTotal()
    {
        return array_reduce($this->items, function ($carry, $item) {
            /** @var $item CartItem */
            return $carry + $item->price * $item->quantity;
        });
    }

    /**
     * Adds or updates a product in the shopping cart
     *
     * @param mixed $identifier
     * @param array $data
     * @param float $price
     * @param float $qty
     */
    public function add($identifier, $data, $price, $qty = 1.0)
    {
        $index = $this->indexOf($identifier);

        // In case item is not found, add it. Else update quantity
        if ($index == -1) {
            $this->items[] = new CartItem($identifier, $qty, $price, $data);
        } else {
            $this->items[$index]->quantity += $qty;
        }
    }

    /**
     * Removes the product with the specified identifier from the shopping cart
     *
     * @param $identifier
     */
    public function remove($identifier)
    {
        $this->removeItemAt($this->indexOf($identifier));
    }

    /**
     * Updates an item
     *
     * @param $identifier
     * @param $qty
     * @param array $data
     * @throws exceptions\ItemNotFoundException
     */
    public function update($identifier, $qty, $data = null)
    {
        $idx = $this->indexOf($identifier);

        if ($idx === -1) {
            throw new exceptions\ItemNotFoundException(sprintf("Item with identifier '%s' not found", $identifier));
        }

        if ($qty <= 0) {
            $this->removeItemAt($idx);

            return;
        }

        $this->items[$idx]->quantity = $qty;
        if ($data !== null) {
            $this->items[$idx]->data = $data;
        }
    }

    /**
     * Returns the index of the element with the specified identifier
     *
     * @param $identifier
     * @return int
     */
    protected function indexOf($identifier)
    {
        $position = 0;
        foreach ($this->items as $item) {
            if ($item->identifier === $identifier) {
                return $position;
            }
            $position++;
        }

        return -1;
    }

    /**
     * Removes the item at the specified index
     *
     * @param $index
     */
    protected function removeItemAt($index)
    {
        array_splice($this->items, $index, 1);
    }

    /**
     * @param $index
     * @throws exceptions\InvalidIndexException
     * @return CartItem
     */
    protected function getItemAt($index)
    {
        if (array_key_exists($index, $this->items)) {
            return $this->items[$index];
        } else {
            throw new exceptions\InvalidIndexException("Item cannot be found");
        }
    }
}

CartItem.php

<?php

namespace maldoinc\utils\shopping;

/**
 * Class ShoppingCartItem
 * @package maldoinc
 *
 * @property string identifier
 * @property float quantity
 * @property float price
 * @property array data
 */
class CartItem implements \Serializable
{
    public $attr = array(
        'identifier' => null,
        'quantity' => null,
        'price' => null,
        'data' => null
    );

    /**
     * @param $identifier
     * @param $quantity
     * @param $price
     * @param $data
     */
    public function __construct($identifier, $quantity, $price, $data)
    {
        $this->identifier = $identifier;
        $this->quantity = $quantity;
        $this->price = $price;
        $this->data = $data;
    }

    protected function hasProp($name)
    {
        return array_key_exists($name, $this->attr);
    }

    protected function checkProp($name)
    {
        if (!$this->hasProp($name)) {
            throw new exceptions\InvalidPropertyException(sprintf("Invalid property: %s on class %s", $name, __CLASS__));
        }
    }

    public function __get($name)
    {
        $this->checkProp($name);

        return $this->attr[$name];
    }

    public function __set($name, $val)
    {
        $this->checkProp($name);

        if ($name === 'quantity' && (float)$val <= 0) {
            throw new exceptions\InvalidQuantityException(sprintf("Invalid quantity: %.2f", $val));
        }

        $this->attr[$name] = $val;
    }

    public function __isset($name)
    {
        return isset($this->attr[$name]);
    }

    /**
     * (PHP 5 &gt;= 5.1.0)<br/>
     * String representation of object
     * @link http://php.net/manual/en/serializable.serialize.php
     * @return string the string representation of the object or null
     */
    public function serialize()
    {
        return serialize($this->attr);
    }

    /**
     * (PHP 5 &gt;= 5.1.0)<br/>
     * Constructs the object
     * @link http://php.net/manual/en/serializable.unserialize.php
     * @param string $serialized <p>
     * The string representation of the object.
     * </p>
     * @return void
     */
    public function unserialize($serialized)
    {
        $this->attr = unserialize($serialized);
    }
}

Answer 1

This seems pretty nice. I have a couple of minor tips though.

---

There are a few $this->intf !== null checks here and there. To avoid such noise, perhaps instead of null as the default value, you could use a sort of "NullInterface" implementation that simply does nothing when the persistence operations are called. Then you could remove all these null checks.

---

Many of the methods of Cart don't need the index of an item, but they access them through indexOf calls and checking the result of that against -1, essentially a magic number. That is, the methods get, add, update. Despite a little code duplication, it might be slightly cleaner to implement get without using indexOf, and implement add and update in terms of get, checking against null values instead of the magic number -1.

protected function get($identifier)
{
    foreach ($this->items as $item) {
        if ($item->identifier === $identifier) {
            return $item;
        }
    }

    return null;
}

---

I'm wondering if this part could be done differently:

    $items = $this->intf->load();

    if ($items !== null) {
        $this->setItems(unserialize($items));
    }

If possible, it would be better to require implementations of intf->load to return values that are ready to use without null checks. A common practice for methods that are expected to return a collection is to return an empty collection and never null.

---

A very minor thing, I'd drop the else here:

    if (array_key_exists($index, $this->items)) {
        return $this->items[$index];
    } else {
        throw new exceptions\InvalidIndexException("Item cannot be found");
    }

to write like this instead:

    if (array_key_exists($index, $this->items)) {
        return $this->items[$index];
    }
    throw new exceptions\InvalidIndexException("Item cannot be found");

Answer 2

Janos allready lists some great improvements. I want to just add some things:

---

Instead of all that indexOf magic. Why not index the $this->items by the identifier? On add:

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

and get:

return $this->items[$identifier];

---

I also don't like that your Car knows how to store it's items. It should not care if the items are stored serialized, or as json, or... That is the task of the CartPersistentInterface. But then again, the Cart should not be aware of the existence of such an interface. It is the task of the controlling code to persist the cart and it should happen outside of the cart.

A cart itself has nothing to do with saving, retrieving, ... It's sole purpose is to be a cart.