0
\$\begingroup\$

I have implemented a collection class that stores a single type of value.

At the moment it is immutable.

abstract class Collection implements Iterator
{

    /** @var int */
    protected $position = 0;
    /** @var array */
    private $items = [];

    public function next()
    {
        ++$this->position;
    }

    public function key()
    {
        return $this->position;
    }

    public function valid()
    {
        return isset($this->items[$this->position]);
    }

    public function rewind()
    {
        $this->position = 0;
    }

    protected function add($item)
    {
        return $this->items[] = $item;
    }

    protected function get($index)
    {
        return $this->items[$index];
    }
}

DateTimeIterator guarantees that when iterated over it will only return the specified type.

interface DateTimeIterator extends Iterator
{

    public function current(): DateTimeInterface;
}

The constructor of DateTimeCollection ensures that the collection only stores values that implement DateTimeInterface.

class DateTimeCollection extends Collection implements DateTimeIterator
{

    public function __construct(iterable $values = [])
    {
        foreach ($values as $value) {
            if (!$value instanceof DateTimeInterface) {
                throw new TypeError('DateTimeCollection only accepts ' .
                    'items that implement DateTimeInterface');
            }
            $this->add($value);
        }
    }

    public function current(): DateTimeInterface
    {
        return $this->get($this->key());
    }
}
\$\endgroup\$
2
\$\begingroup\$

The Collection is not really immutable if you can add items to it :)


You should explicitly define either abstract or simple default implementation (i.e. that doesn't enforce return data type) for current() method on Collection.


I would suggest using $this->key() within valid()

public function valid()
{
    return isset($this->items[$this->key()]);
}

This is consistent with your usage in current() in inheriting classes and consistent with iterator interface. This also allows for override of key() in inheriting classes if there ever was such a need for this.


Consider explicitly specifying abstract constructor in Collection class (or again a simple default constructor (i.e. just setting position to O)). You should explicitly set position to 0 in constructor in some manner.


You might consider exposing a length() function on your collection since you are providing functionality beyond basic iteration. This would allow you to do things like perhaps return new index value to caller on add().

public function length() {
    return count($this->items[]);
}

protected function add($item)
{
    $this->items[] = $item;
    // return index of new item
    return $this->length() - 1;
}

You also should consider validating input parameter passed to get against valid method as being within range of keys.

protected function get($index)
{
    if(!$this->valid($index)) {
        throw new OutOfBoundsException(
            'Index passed to get() is invalid.'
        );
    }
    return $this->items[$index];
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.