auraphp / Aura.Payload

A Domain Payload implementation.
MIT License
55 stars 18 forks source link

Shouldn't we be using a Factory and not a single Payload? #7

Closed jakejohns closed 9 years ago

jakejohns commented 9 years ago

It seems to me that each call to the service should return it's own new payload, and not simply work with a shared instance. If this is correct, I'd think the injection of a factory would be more appropriate. In this case, I think the package should provide one and the documentation should suggest it so as to influence good practice.

It is my understanding that the Payload is considered to be a data transfer object for the Domain. The implementation outlined in the documentation seems, to me, unnecessarily influenced by the paradigm suggested by ADR. Specifically, it seems to suggest that the service object will only receive a single call, eg. a route dispatching an action calling a single method on the domain.

This implementation seems to imply that for a consuming UI to make multiple calls to the service, it must instantiate new instances for each call. Perhaps I am misunderstanding something though.

Minimal Example:

class SomeService
{
    protected $payload;

    public function __construct(Payload $payload)
    {
        $this->payload = $payload;
    }

    public function __invoke($foo)
    {
        if ($this->doSomething($foo)) {
            return $this->payload
                ->setStatus(Payload::VALID)
                ->setExtras(['foo' => 'foo does something extra']);
        }

        return $this->payload->setStatus(Payload::VALID);
    }
}

class SomeUI
{
    protected $service;

    public function __construct(SomeService $service)
    {
        $this->service = $service;
    }

    public function __invoke($array)
    {
        foreach ($array as $foo) {
            $payload = $this->service->__invoke($foo);

            // If SomeService::doSomething is true once,
            // all subsequent payloads have "extras"
            if ($extras = $payload->getExtras()) {
                $this->doSomethingExtra($extras);
            }
        }
    }
}

Certainly, one could be sure to "clear the state" of the payload every time, or make sure that all setters are always called if they are pertinent, but this seems onerous and incorrect.

Also, it is certainly true that the implementing code can trivially implement their own factory. However, it seems that suggesting this may be better, as it seems more correct/functional to me.

Let me know if I'm wrong. Thanks!

pmjones commented 9 years ago

An excellent point. Arguably, the Payload object is so dead-stupid simple that the examples could get away with a Prototype instead, and clone the prototype for modification. Thoughts?

jakejohns commented 9 years ago

That (using Prototype) is actually what I'm currently doing, but had a (possibly irrational) doubt.
It's probably just because I feel like Factory is usually (?) the right choice.

I can't actually think of a reason Prototype wouldn't work here. Also, when raised in IRC, there were a few resounding cheers for Factory, but the opinions seemed generalized and non-specific.

I'm curious what others have to say.

Are there some cases of people using the interface where Factory makes more sense that Prototype? I can't seem to think of a good one off hand.

pmjones commented 9 years ago

Well, adding a Factory option certainly can't hurt. That allows for single-use, prototype, factory method, and factory object. Accepted!