I'd say follow the Single Responsibility Principle.
What you have is a SportsCard that does nothing on itself. It's just a data container.
class SportsCard
{
protected $_price;
protected $_sport;
public function __construct($price, $sport)
{
$this->_price = $price;
$this->_sport = $sport;
}
public function getPrice() { return $this->_price; }
public function getSport() { return $this->_sport; }
}
The SportsCards have to be stored in a CardBook. If you think about a CardBook, then it doesn't do much, except for storing SportsCards (not Pokemon or Magic The Gathering cards), so let's just make it do that:
class CardBook extends SplObjectStorage
{
public function attach($card)
{
if ($card instanceof SportsCard) {
parent::attach($card);
}
return $this;
}
public function detach($card)
{
parent::detach($card);
}
}
While there is nothing wrong with adding Finder Methods to the SportsCard set, you do not want the finder method's logic inside it as well. It is fine as long as you only have
public function findByPrice($min, $max);
public function findBySport($sport);
But the second you also add
public function findByPriceAndSport($sport, $min, $max);
you will either duplicate code or iterate over the collection twice. Fortunately, PHP offers an elegant solution for filtering collections with Iterators, so let's put the responsibility for filtering into separate classes as well:
abstract class CardBookFilterIterator extends FilterIterator
{
public function __construct($cardBook)
{
if ($cardBook instanceof CardBook || $cardBook instanceof self) {
parent::__construct($cardBook);
} else {
throw new InvalidArgumentException(
'Expected CardBook or CardBookFilterIterator');
}
}
}
This class merely makes sure the child classes accept only children of itself or CardBooks. This is important because we are accessing methods from SportsCard in actual filters later and because CardBooks can contain only SportsCards, we make sure the iterated elements provide these methods. But let's look at a concrete Filter:
class SportCardsBySportFilter extends CardBookFilterIterator
{
protected $sport = NULL;
public function filterBySport($sport)
{
$this->_sport = $sport;
return $this;
}
public function accept()
{
return $this->current()->getSport() === $this->_sport;
}
}
FilterIterators work by extending the class and modifying it's accept() method with custom logic. If the method does not return FALSE, the currently iterated item is allowed to pass the filter. Iterators can be stacked, so you add Filter on Filter on top, each caring only for a single piece of filtering, which makes this very maintainable and extensible. Here is the second filter:
class SportCardsByPriceFilter extends CardBookFilterIterator
{
protected $min;
protected $max;
public function filterByPrice($min = 0, $max = PHP_INT_MAX)
{
$this->_min = $min;
$this->_max = $max;
return $this;
}
public function accept()
{
return $this->current()->getPrice() > $this->_min &&
$this->current()->getPrice() < $this->_max;
}
}
No magic here. Just the accept method and the setter for the criteria. Now, to assemble it:
$cardBook = new CardBook;
$cardBook->attach(new SportsCard('10', 'Baseball'))
->attach(new SportsCard('40', 'Basketball'))
->attach(new SportsCard('50', 'Soccer'))
->attach(new SportsCard('20', 'Rugby'))
->attach(new SportsCard('30', 'Baseball'));
$filteredCardBook = new SportCardsBySportFilter($cardBook);
$filteredCardBook->setFilterBySport('Baseball');
$filteredCardBook = new SportCardsByPriceFilter($filteredCardBook);
$filteredCardBook->filterByPrice(20);
print_r( iterator_to_array($filteredCardBook) );
And this will give:
Array (
[4] => SportsCard Object (
[_price:protected] => 30
[_sport:protected] => Baseball
)
)
Combining filters from inside the CardBook is a breeze now. No Code duplication or double iteration anymore. Whether you keep the Finder Methods inside the CardBook or in a CardBookFinder that you can pass CardBooks into is up to you.
class CardBookFinder
{
protected $_filterSet;
protected $_cardBook;
public function __construct(CardBook $cardBook)
{
$this->_cardBook = $this->_filterSet = $cardBook;
}
public function getFilterSet()
{
return $this->_filterSet;
}
public function resetFilterSet()
{
return $this->_filterSet = $this->_cardBook;
}
These methods just make sure you can start new FilterSets and that you have a CardBook inside the Finder before using one of the finder methods:
public function findBySport($sport)
{
$this->_filterSet = new SportCardsBySportFilter($this->getFilterSet());
$this->_filterSet->filterBySport($sport);
return $this->_filterSet;
}
public function findByPrice($min, $max)
{
$this->_filterSet = new SportCardsByPriceFilter($this->getFilterSet());
$this->_filterSet->filterByPrice(20);
return $this->_filterSet;
}
public function findBySportAndPrice($sport, $min, $max)
{
$this->findBySport($sport);
$this->_filterSet = $this->findByPrice($min, $max);
return $this->_filterSet;
}
public function countBySportAndPrice($sport, $min, $max)
{
return iterator_count(
$this->findBySportAndPrice($sport, $min, $max));
}
}
And there you go
$cardBookFinder = new CardBookFinder($cardBook);
echo $cardBookFinder->countBySportAndPrice('Baseball', 20, 100);
$cardBookFinder->resetFilterSet();
foreach($cardBookFinder->findBySport('Baseball') as $card) {
echo "{$card->getSport()} - {$card->getPrice()}";
}
Feel free to adapt and improve for your own CardBook.