Entity collection best practice (separation of concerns, dependency injection, ...)

Question

I build a collection to ensure only valid entities in an array so I don't have to validate after every function / method call as I could rely on the collection to have only valid entities stored.

class WorldCollection implements Collection, ArrayAccess {

    private $container = array();
    private $oValidator;

    public function __construct (Validator $oWorldValidator) {
        $this->oValidator = $oWorldValidator;
    }

    public function offsetExists ($offset) {
        return isset($this->container[$offset]);
    }


    public function offsetGet ($offset) {
        return isset($this->container[$offset]) ? $this->container[$offset] : null;
    }

    /**
     * READ ME
     */    
    public function offsetSet ($offset, $value) {
        $this->oValidator->validate($value)
        if (is_null ($offset)) {
            $this->container[] = $value;
        } else {
            $this->container[$offset] = $value;
        }
    }

    public function offsetUnset ($offset) {
        unset($this->container[$offset]);
    }
}

As I want to separate concerns and follow dependency injection I inject the Validator in the constructor of the collection and validate in the ofsetSet method to ensure only valid values will be stored.

class WorldValidator implements Validator {

    public function validate($aWorld) {
        if (!is_array($aWorld)) {
            throw new WorldValidationException('invalid type', WorldValidationException::ERROR_CODE_INVALID_TYPE);
        }

        return true;
    }
}

(The validator is simplified for example purpose.)

1. Did I seperate it right? Or should I handle this kind of validation differently?
2. Should I catch the validation exception to not disturb the program flow? If so should I store the exception and offer a getErrors method?
3. Should I rename the collection ValidWorldCollection and extend from WorldCollection?
4. Should the InvalidWorldException extend RuntimeException or LogicException? Same for WorldValidationException in the validator
5. Should I create a World Class and exclude the Validation from the Collection using instanceof ?
6. Should I have used a more appropriate Spl DataStructure? (SplObjectStorage?) Or is it all a waste of time?

Answer 1

Design

Dependency injection is very useful when you have multiple implementations of a given dependency. If you don't plan implementing another Validator, then it's probably premature abstraction. I'll assume here that you have other Validator implementations.

Let me answer your questions:

1. The separation itself is good.
2. An exception is meant to be catched "whenever possible", and it is not possible in WorldCollection, since you don't know how to handle it. You need to wonder what you want your application to do when the data does not validate.

If you want WorldCollection::offsetSet to not do anything when the validation fails, then WorldValidator::validate should simply return true or false, and no exception should be thrown.

If the exception is to be catched later on in the code, then don't do anything in WorldCollection::offsetSet, since another code will take care of the issue.

You can easily mix those two approaches in order to catch some of the exception at some point, and other ones later on (which means WorldValidator::validate should always throw an exception, and return nothing).

3. Only if you plan on having other WorldCollections. I don't think so.
4. LogicException should only be launched if there is a flow in your program, ie. an exception that would never be thrown in a perfect program. RuntimeException is a base class used for errors such as OutOfBounds or Overflow. I'd use InvalidArgumentException.
5. I didn't understand the question. Note that instanceof is never good design, and exists to work around design errors.
6. ArrayAccess is a reasonable choice, it lets you do foreach on your data easily, whereas with Iterator you would have to implement a ton of methods. SplObjectStorage could be used, but only if you don't need the keys ArrayAccess provides.

Implementation

1. Do you want only one exception with different types, such as ERROR_CODE_INVALID_TYPE?
2. Why don't you use multiple exceptions?
3. If you want to stick with one exception, don't pass "invalid type" in the constructor, since it is always going to be the same message. Prefer to override Exception::__toString() to return "invalid type" when the type is ERROR_CODE_INVALID_TYPE.
4. Use offsetExists in offsetGet instead of repeating yourself?
5. Consider using NULL instead of null, and TRUE instead of true, even if they are case-insensitive.
6. $this->oValidator->validate($value) forgot a semicolon?
7. Don't use return if you're not going to check the return value.