Sixteen Concepts, Copied Once

Extracting a module out of another one leaves a wake. You can read it afterwards like tree rings, and in GroupFlights the reading is unusually clear because the extraction is documented: ADR 04 records the decision to lift reservation changes out of Sales into a satellite module, with two explicit recommendations — high encapsulation of the domain model, and a Supplier-Customer relationship in which Sales is the more important party.

Part 16 finished the framework material. This part reads the wake. Sixteen concepts exist twice in this repository, once in each module, and the duplicates are not the interesting part. The divergences are.

The inventory

Add a seventeenth if you count CreateFlightSegment, a six-line mapper that appears five times in Sales and twice in Postsale.

Before the criticism: most of this duplication is the intended cost of module autonomy. ADR 01 mandates shared contracts, not shared models, and that is the correct call. A Shared.Domain project holding FlightSegment for both modules would couple two bounded contexts at exactly the layer the whole architecture exists to keep separate. Duplication across a context boundary is not a smell; it is the boundary working.

The question worth asking is not “why is this duplicated” but "why is this copy different?" — because each difference marks a decision the extraction forced.

Divergence one: a value object grew an identity

// src/Sales/GroupFlights.Sales.Domain/Reservations/Changes/NewTotalCost.cs
public record NewTotalCost(Money TotalCost, Money RefundableCost);

// src/Postsale/GroupFlights.Postsale.Domain/Changes/Outcome/ReservationCost.cs:5-11
public record ReservationCost(Money TotalCost, Money RefundableCost, Guid Id)
{
    private ReservationCost() : this(default, default, default)
    {
    }
}

Same two values, plus a Guid Id. Nothing in the domain uses it — no repository looks a cost up by id, no rule compares them, no event carries it as a correlation. It exists because OwnsOne needed something to key on, and the domain service dutifully mints one every time it maps a cost across the boundary.

That is a persistence concern reaching back into a value object and giving it identity, which is precisely what value objects are defined by not having. You can see the whole story in a two-line diff, and the column it produced (_newCost_Id) is in the schema from Part 8.

Divergence two: getters that became private setters

// Sales                              // Postsale
public ushort Hours { get; }          public ushort Hours { get; private set; }
public ushort Minutes { get; }        public ushort Minutes { get; private set; }

Otherwise the two FlightTime classes are character-for-character identical, down to the two ArgumentOutOfRangeException guards and the blank line inside the private constructor.

{ get; } compiles to a readonly backing field. EF Core can write it during materialisation in most configurations, but inside a nested owned type reached through OwnsMany it is fragile enough that somebody changed it. Sales' copy could stay immutable because Newtonsoft's forced-writable resolver (Part 6) writes readonly fields regardless.

Two persistence strategies, two different amounts of immutability the model is allowed to keep. That is a cost of the JSON approach that lands in Sales' favour, and it is the only entry on that side of the ledger.

Divergence three: the strongly-typed id that was dropped

// Sales
public record Deadline(DeadlineId Id, DateTime DueDate, bool? Fulfilled = null);
public record DeadlineId(Guid Value);

// Postsale
public record Deadline(Guid Id, DateTime DueDate, bool? Fulfilled = null)
{
    private Deadline() : this(default, default, default) { }
}

Sales invented four strongly-typed ids — OfferId, ReservationId, AirlineOfferId, DeadlineId — with implicit conversions, and uses them consistently. Postsale uses raw Guid throughout, in every id position, in every record.

The reason is visible in the mapping: HasConversion for a wrapper type is straightforward at the top level and genuinely painful for a wrapper nested inside an owned type inside an owned collection. The estate's _requester conversion (UserId to nullable Guid) shows the shape, and it takes four lines with explicit null handling on both legs. Doing that for every nested id would have doubled the config file.

So a good practice was adopted in one module and abandoned in the next, and the reason is EF ergonomics rather than a design position. The consequence is a live hazard at the seam: the Postsale integration event is (Guid ReservationId, Guid ReservationChangeRequestId, …) while its source domain event is ReservationChangeAccepted(Guid ReservationChangeRequestId, Guid ReservationId, …) — two adjacent Guid parameters in opposite orders, one mapping apart, with no compiler protection. Both current call sites happen to be correct.

The divergence that is load-bearing

Two files, same class name, same namespace suffix, twelve lines each.

// src/Sales/GroupFlights.Sales.Domain/Shared/DomainEvents/Base/DomainEventsSource.cs:6-14
public IReadOnlyCollection<IDomainEvent> DomainEvents
{
    get
    {
        var result = _domainEvents.ToList();
        _domainEvents.Clear();          // drains
        return result;
    }
}

// src/Postsale/GroupFlights.Postsale.Domain/Shared/Base/DomainEventsSource.cs:6-13
public IReadOnlyCollection<IDomainEvent> DomainEvents
{
    get
    {
        var result = _domainEvents.ToList();
        return result;                  // does not drain
    }
}

One line. Opposite contracts.

Sales' version is a property that mutates on read — a textbook violation of Command-Query Separation, and a genuinely dangerous one. A debugger watch window evaluating offer.DomainEvents silently discards every pending event. So does a log line. So does an added assertion in a test. The method wants to be called DequeueEvents(), and if it were, nobody would ever put it in a watch expression.

Postsale's version is the safe one. Somebody removed the Clear() during the extraction — deliberately, one assumes, because it is a single line.

And then the repository started depending on it:

// src/Postsale/.../Repositories/ReservationChangeRequestRepository.cs:72-85
public async Task Update(ReservationChangeRequest reservationChangeRequest, CancellationToken cancellationToken = default)
{
    _dbContext.ChangeRequests.Update(reservationChangeRequest);

    var changeAcceptEvent = reservationChangeRequest
        .DomainEvents.OfType<ReservationChangeAccepted>().SingleOrDefault();

    if (changeAcceptEvent is not null)
    {
        _dbContext.ChangesToApply.Add(changeAcceptEvent.ReservationChangeToApply);
    }

    await _dbContext.SaveChangesAsync(cancellationToken);
}

The repository reads the event queue to discover whether a child entity needs inserting — a direct consequence of the sealed aggregate from Part 4, which will not tell it any other way. And then the handler publishes:

// src/Postsale/.../EventHandlers/External/PaymentCompletedEventHandler.cs:29-35
changeRequest.OnChangePayed();

await _repository.Update(changeRequest, cancellationToken);

await _eventDispatcher.PublishMultiple(changeRequest.DomainEvents
    .Select(@event => @event.RemapToPublicEvent())
    .SelectMany(e => e), cancellationToken);

Two reads of DomainEvents, in sequence, both of which must see the same events. Under Sales' draining version, the second read returns nothing and the change is never announced. The repository peeks, the publisher publishes, and the whole thing works only because of one line somebody deleted in a base class that looks identical at a glance.

That behavioural dependency is documented nowhere, tested in neither module, and would be reintroduced by anyone who noticed the two files had drifted and “fixed” it by making them match.

Two files that look the same and behave differently are worse than two files that look different, because the reviewer's eye stops at the first one. If your copies must diverge, make the divergence loud — different member names, a comment naming the dependency, or, best, one method called DequeueEvents() and another called PeekEvents() so the ambiguity cannot exist.

The archaeology

Three more artefacts, worth a paragraph because they show the extraction mid-move rather than finished:

• Sales.Domain/Reservations/Changes/ holds five files mirroring Postsale.Domain/Changes/Outcome/ one for one. Sales kept the apply half of the change model; Postsale took the decide half. Neither folder name says so.
• Two exception types declared in Sales and never thrown — CannotChangeTravelDestinationAfterTicketsIssuedException and TravelDateChangesMustCorrectDeadlinesException. Both describe rules that now live, or should live, in Postsale.
• Postsale.Domain/Changes/Specifications/IsDeadlineOverdue came across with the module and has zero callers. A specification copied for a rule the new module does not enforce.

Read together, they say something useful about extraction: the code moved cleanly, and the naming did not follow it. Every one of those is a five-minute rename that nobody had a reason to do, because the compiler was happy and the tests — all six of them — were unaffected.

Next, the retrospective: what teaching code owes its reader.