Protecting invariants when creating value objects through Entity or Factory

[webdevilopers]

My original post and poll started here:
https://twitter.com/webdevilopers/status/1100102866583339008

This was the original gist:

• https://gist.github.com/webdevilopers/fa3bbfbb8e2b36a4a4619217e6469fdc

Use case:
I need to calculate a Dormer based on specific DormerType (Entity).
Business rule: Some DormerTypes can not have a Gutter attached.
Dormer in the end will just be a value object stored inside an DormerCalculation Entity. / Aggregate Root that could throw the DormerCalculated Domain Event.
This means that the DormerTypeId should only be a reference.

I currently think of three approaches.

Version 1:

The full Entity is passed to the value object. This feels ugly. Allthough it is now impossible for a developer to create an inconsistent object. E.g. by attaching a gutter to a dormer with a type that does not allow gutters.

<?php

final class Dormer
{
    /** @var DormerType $type */
    private $type;

    public static function construct(DormerType $type)
    {
        $this->type = $type;
    }

    public function addGutter(Gutter $gutter)
    {
        if (!$this->type->canHaveGutterInstalled()) {
            throw new DormerTypeCannotHaveGutterInstalledException();
        }
 
       $this->gutter = $gutter;
    }
}

final class DormerCalculationFactory extends AbstractCalculator
{
    private $dormerTypeRepository;

    public function createWithDto(CalculateDormerCommand $dto)
    {
        $dormerType = $this->dormerTypeRepository->ofId(DormerTypeId::fromInteger($dto->dormerTypeId));

        /** @var Dormer $dormer */
        $dormer = Dormer::construct($dormerType);
        $dormer->addGutter(Gutter::fromDto($dto->gutter));
    }
}

Version 2:

Again the business rule is ensured. The value object is created through the dormer type Entity which sounds logical. But in the end it is not an Aggregate Root that e.g. could throw a "DormerCalculated" event.
In addition the factory method calculateDormer and the factory method on the dormer value object would look too identical.

<?php

class DormerType()
{
    private $id;

    public function calculateDormer(Gutter $gutter)
    {
         if (!$this->canHaveGutterInstalled()) {
            throw new DormerTypeCannotHaveGutterInstalledException();
        }
       
        return new Dormer($this->id(), $gutter);
    }
}

Version 3:

All creation logic is inside the factory. It protects the invariants and creates the Dormer only with the DormerTypeId.
But a developer could create an inconsistend value object by skipping the factory. Ouch!?

final class DormerCalculationFactory extends AbstractCalculator
{
    private $dormerTypeRepository;

    public function createWithDto(CalculateDormerCommand $dto)
    {
        $dormerType = $this->dormerTypeRepository->ofId(DormerTypeId::fromInteger($dto->dormerTypeId));

        /** @var Dormer $dormer */
        $dormer = Dormer::construct($dormerType->id());

        if (null !== $dto->gutter && $dormerType->canHaveGutterInstalled()) {
            $dormer->addGutter(Gutter::fromDto($dto->gutter));
        }
    }
}

After reading a post on @culttt b @philipbrown version 3 still feels like the best way though.

Due to the coupled nature of the Factory and the object, it is usually fine to allow the Factory to protect the invariants of the object.

• https://www.culttt.com/2014/12/24/factories-domain-driven-design/

What do you guys think?

[webdevilopers]

The remaining classes and interfaces:

interface DormerTypeInterface
{
    public function canHaveGutterInstalled(): bool;
}

abstract class DormerType implements DormerTypeInterface
{}

class FlatRoofDormer extends DormerType
{
    public function canHaveGutterInstalled(): bool
    {
        return true;
    }
}

[yvoyer]

I would go with Version 1, but define the DormerType argument as an interface. I know it feels ugly to pass the entity to the value object, but in reality, you are passing an interface. So you could test your Dormer class with a double that always return true or false.

public function testItShouldAllowToAddGutter() 
{
    $dormer = Dormer::withType(new AlwaysAllowAddingGutter());// canHaveGutterInstalled() always return true 
    $dormer->addGutter($this->createMock(Gutter::class));
    ....
}

public function testItDoNotAllowToAddGutter() 
{
    $dormer = Dormer::withType(new NeverAllowAddingGutter()); // canHaveGutterInstalled() always return false

    $this->expectException(DormerTypeCannotHaveGutterInstalledException::class);
    $dormer->addGutter($this->createMock(Gutter::class));
    ....
}

Version 2: You could also use it, but instead of defining the DormerType as a class, use an interface again.
Version 3: I do not like it, since as you said it, it allows developers to create the object without checking invariant

Note: if you want the Dormer to be a value object, it should not have state and should be immutable.

final class Dormer
{
    private $type;
    private $gutters = [];
    private function __construct(DormerType $type, Gutter ...$gutters) {
        $this->type = $type;
        $this->gutters = \array_map(
            function (Gutter $gutter) {
                $this->addGutter($gutter); // this makes sure the check to add gutter is respected
            },
            $gutters
        );
    }
    public function addGutter(Gutter $gutter): Dormer 
    { 
         if (!$this->type->canHaveGutterInstalled()) {
            throw new DormerTypeCannotHaveGutterInstalledException();
        }
        
        return new self($this->dormerType, \array_merge([$gutter], $this->gutters));
    }

    public static function withType(DormerType $type): self {
        return new self($type);
    }
}

[yvoyer]

As a side note: It feels like the DormerCalculationFactory is more of a handler than a factory. IMO, a factory would created an object, where the current implementation seems to add a gutter. Maybe i'm missing something, but then I don't know your domain in depth.

With my previous comment, the factory would become a handler:

final class CalculateDormerHandler
{
    private $dormerTypeRepository;

    public function __invoke(CalculateDormerCommand $dto)
    {
        $dormerType = $this->dormerTypeRepository->ofId(DormerTypeId::fromInteger($dto->dormerTypeId));
        $dormer = Dormer::withType($dormerType);
        $dormer->addGutter(Gutter::fromDto($dto->gutter));
        // do we save the type? what state di we changed?
    }
}

[webdevilopers]

Thanks again @yvoyer for this comprehensive feedback.

Indeed my factory currently holds more complex logic. It bloated the original Application Handler. That's why I moved it to a "Factory".

One thing about creating the "Dormer" VO through the "DormerType" Entity:
Would the returned "Dormer" VO then only hold the "DormerTypeId" instead of the full Entity?
Just wondering if this is a normal approach: passing full Entities to a (factory) method but only adding the ID to the resulting project.

[yvoyer]

The problem if you pass only the id, is that you lose the option to check whether a gutter can be added, since the DormerTypeId do not know about the DormerType.

As I said, you are not passing an entity to the value object, but an interface DormerType which in the application is implemented by your entity. From the value object point of view its just an interface, since it do not know about an entity.

If you fear that passing the entity would be bad, create an adapter that wraps the entity while implementing the DormerType interface, but IMO, its not necessary since the entity can do it itself.

Having a better view at the transaction's process would be easier to help you.

[webdevilopers]

I'm fine with passing the DormerType to Dormer. i just wanted to ensure that it is ok to pass it but then let the newly created Object only keep the DormerTypeId instead of the complete DormerType.

Since the Dormer Value Object will be persisted inside the DormerCalculation Aggregate Root.

[yvoyer]

But you will probably need the type to be stored in the Dormer since you need to check the condition on addGutter(). For the persistence on the aggregate, you can add the method Dormer::getType(): DormerTypeId to extract the id to be used by the aggregate.

// aggregate
public function calculateDormer(Dormer $dormer): void 
{
    $this->dormer = $dormer;
    $this->dormerType = $dormer->getType(); // DormerTypeId
}

Does it make sense?

[webdevilopers]

Absolutely. BTW I already used an interface but honestly did not recognize how easy it will make mocking when testing until you pointed it out. ;)

Maybe @matthiasnoback and @Ocramius also have a little feedback on this topic? :)

[webdevilopers]

The reason I originally liked the Version 2 was an example of the red book by @VaughnVernon:

    public Discussion startDiscussionFor(
            ForumIdentityService aForumIdentityService,
            Author anAuthor,
            String aSubject,
            String anExclusiveOwner) {

        if (this.isClosed()) {
            throw new IllegalStateException("Forum is closed.");
        }

        Discussion discussion =
                new Discussion(
                    this.tenant(),
                    this.forumId(),
                    aForumIdentityService.nextDiscussionId(),
                    anAuthor,
                    aSubject,
                    anExclusiveOwner);

        return discussion;
}

• https://github.com/VaughnVernon/IDDD_Samples/blob/master/iddd_collaboration/src/main/java/com/saasovation/collaboration/domain/model/forum/Forum.java#L153-L173

Not being able to create a Discussion for a closed Forum is similar to my use case where you can not at a Gutter to DormerType that does not support it. See Version 2.

But any developer could still skip the Forum Entity invariant protection and simply create the Discussion Entity in order to persist it:

        Discussion discussion =
                new Discussion(
                    aTenant,
                    aForumId,
                    aForumIdentityService.nextDiscussionId(),
                    anAuthor,
                    aSubject,
                    anExclusiveOwner);

Or am I missing something?

I guess my main concern form a developer team point-of-view is:
Does "protecting invariants" mean that I should:

• offer my developer team a way to create objects in a valid state though they could "skip" them
• force my developer to use the right way and prevent using any other way

?