AbstractCollection constructor overrides AbstractArray constructor behavior about collection HOT 10 CLOSED

I understand your argument, and in some ways I agree, but the point of the AbstractCollection is to enforce a type on the collection.

I suppose I could change the constructor to:

public function __construct(array $data = [], $collectionType)

The class would need to set the $collectionType first and then iterate over each item in $data to add them to the collection, enforcing the type check for each one.

What do you think?

from collection.

I understand your argument, and in some ways I agree, but the point of the AbstractCollection is to enforce a type on the collection.

You are absolutely right. I'm just asking about where this policy could be declared. If you are considering the change of the constructor, why don't drop the hole constructor?

The need to add the type in the constructor is because $collectionType is private and not protected. There are other approaches like:

making $collectionType protected so when a collection is derivated from AbstractCollection it could include the type on its own specification. No need to write the constructor, then it will be inherith from AbstractArray and the object will be populated already.

// FooCollection will allow only members of Foo
class FooCollection extends AbstractCollection
{
    // allow only Foo members on this class
    protected $collectionType = Foo::class;
}

The downside about this approach is that a derivative class can change the property at runtime (as far I understand this is the reason you make it private). But well, If need to change this property at run time you are choosing the wrong tool.

Other way to achieve the same as before is to keep the property $collectionType private but add a method setType($type), the counterpart of getType(). The method would check that all members are according to this policy and then set the property.

Having the $collectionType in the constructor gives to you the advantage to define the type from outside the class. If you create the setType($type) method then you still have this behavior.

class AbstractCollection
{
    // ...
    public function setType($type)
    {
        foreach($this->data as $item) {
            if ($this->checkType($type, $item) === false) {
                throw new \InvalidArgumentException(
                    'The collection has a value that does not match with the type ' . $type . '; value is '
                    . var_export($item, true)
                );
            }
        }
        $this->collectionType = $type;
    }
    // ...
}

// use like this:
class FooCollection extends AbstractCollection
{
    // this would call AbstractArray::__construct as parent
    public function __construct(array $data = [])
    {
        $this->setType(Foo::class);
        parent::__construct($data);
    }
}
$members = []; // array of Foo
$col = new FooCollection($members);

// Also allows the use of the AbstractClass like this
class BarCollection extends AbstractCollection {}

$barcol = new BarCollection();
$barcol->setType(Bar::class); // allows define the type from outside the collection

Have you consider that the constructor receiving the type is an implementation detail?
Maybe It could be sent to Collection::__constructor instead of AbstractCollection.
Are you trying to mimic templates here? $col = new Collection("Foo")

This could be my fault, but my understanding is that AbstractCollection is a template for another class, and Collection is a "Generic" implementation. With that in mind, this could be two different ways to use it:

$foocol = new FooCollection($fooMembers);
$barcol = new Collection(Bar::class, $barMembers); // as you suggest in your constructor

I will be waiting for your comments.

from collection.

Would you like to check this code? https://github.com/eclipxe13/collection/commit/b303974f8609f181d3a3d033f3261656f78bf9a5

What I see is that no need to change the test. I had change the FooCollection and add other two classes to test the different behaviors that affect AbstractCollection.

from collection.

I’ll have to give some thought to this. The goal is to enforce a type on the objects in the collection, much like the Java Collection library.

With your approach, there is no type enforcement upon instantiation, so any objects may be added until a type is explicitly set, at which point an exception could be thrown on any one of the items already set, and the calling code has no way to recover or back out of this state. It just has to consider it somewhat of a fatal runtime error, since there’s no way to handle the case for a single type-mismatched item—it’s all or nothing.

If PHP had generics, there would be no reason to pass in the desired type upon instantiation, but since PHP doesn’t have generics, I tried to come up with an acceptable alternative that would accomplish the goal of generics.

from collection.

If your problem is primarily with the inheritance of AbstractArray and the break between constructor signatures, I can break the inheritance so that AbstractCollection is no longer a type of AbstractArray. To avoid code duplication, I could move some of the common methods into traits.

from collection.

Another idea I just had: What about adding an abstract getType() method to AbstractCollection? This would enforce the contract to child classes, and we'd keep the constructor signature from ArrayAbstract, but the constructor on AbstractCollection would call getType() to set the type for the collection. There would be no setter for the type, and the property would still be private to restrict overriding at runtime. It must be set from a defined class, so setting the type at runtime upon instatiation would be impossible, but the generic Collection would simply be "mixed," so it could support any type.

I think this is a good compromise. What do you think?

from collection.

I like the approach of making getType() as abstract method. Using this approachs:

• AbstractCollection drops its constructor
• $collectionType and getType() implementations goes to Collection class
• Collection class can have the constructor with $collectionType and $data.
• Allow other/outside implementations like "templating" using $collectionType as protected as I suggested

I had cooked the changes, what do you think?
https://github.com/eclipxe13/collection/commits/feature1

from collection.

Hi @ramsey, I had created a new branch with several commits: https://github.com/eclipxe13/collection/commits/contrib (feature1 was renamed) These include what we talked here, a new TypedMap class, MapInterface::keys() : array that mimics java's Map::keysSet(), a trait method to export a variable as string -used when throwing an exception-, and other minor edits.

I'm very interested on use and contribute to your library, please, let me know your thoughts.

from collection.

@eclipxe13 I liked what I was seeing in the original feature1 branch. I'd prefer if you would turn these into pull requests that are smaller and kept to individual new features. It would make it easier for me to review them. Can you submit a pull request just for the getType() functionality we discussed and then a separate pull request for the TypedMap class? If the MapInterface::keys() and Map::keysSet() methods are separate functionality from TypedMap, please submit those in a separate PR, as well.

I appreciate your eagerness to contribute, so please don't let these requests discourage you. I'm not trying to make more work for you. I'm just trying to keep the pull requests focused, so that I can clearly see the differences. I'll try to respond quicker. :-)

Thanks!

from collection.

@ramsey, sorry for the late response, was offline the weekend.
I will split the changes on different branches and send the PR soon.

from collection.