A Smell Audit With Prescriptions

Code review has a vocabulary problem: “this feels off” starts an argument, while a name ends one. That is the whole reason the refactoring.guru catalogue of smells and techniques earns its shelf space — not because the names are clever, but because “Consolidate Conditional Expression” is an instruction where “clean this up” is a mood. Part 10 closed the file on the estate's machine learning; this part opens the field guide. The rules of the audit: every specimen below is verified at a real path in the Pacco source, every smell gets its catalogue name, and every name comes with its prescribed technique. Nothing here is a pile-on — the estate's surface is unusually clean (there is not a single TODO, HACK or FIXME comment across all thirteen repositories, which I have grep-verified and still find remarkable) — and that is precisely what makes it a good specimen tray. The smells that remain are the quiet kind that survive polish.

Exhibit one: the search that can never find

The headline specimen lives in Vehicles, and it takes three files to see. First, the domain modelling, which is genuinely nice — a [Flags] enum with bit-shift literals:

// Pacco.Services.Vehicles.Core/Entities/Variants.cs
[Flags]
public enum Variants
{
    Standard =  1 << 0,
    Chemistry = 1 << 1,
    Weapon =    1 << 2,
    Animal =    1 << 3,
    Organ =     1 << 4
}

Second, a detail hiding in Vehicle's constructor chain: the params Variants[] constructor delegates to the base constructor, and the base constructor ends with AddVariants(Variants.Standard). Every construction path runs it, so every vehicle in the system carries the Standard bit, whatever variants were requested — an invariant established by constructor chaining, documented nowhere.

Third, the query. SearchVehiclesHandler in Pacco.Services.Vehicles.Infrastructure/Mongo/Queries/Handlers:

pagedResult = await _repository.BrowseAsync(v => v.PayloadCapacity >= query.PayloadCapacity
                                                 && v.LoadingCapacity >= query.LoadingCapacity &&
                                                 v.Variants == query.Variants, query);

v.Variants == query.Variants — exact equality on a flags field. Search for Weapon and the filter demands vehicles whose variants are exactly Weapon: no Standard, no anything else. But the constructor just guaranteed no such vehicle can exist. Any single-variant search matches nothing, forever; the only searches that can succeed are ones that happen to spell out a vehicle's complete combination, Standard included. The guard on the other branch, query.Variants <= 0, quietly treats the enum as an ordinal, which is how the bug slipped past the type system in the first place.

The catalogue name for the predicate's condition is a complex conditional; the prescription is Decompose Conditional — pull the variant test into a named, intention-revealing predicate — and the correct test is a bitmask containment:

private static bool CoversRequestedVariants(VehicleDocument v, Variants requested)
    => (v.Variants & requested) == requested;

with the honest caveat that this is not a one-line fix in situ: the predicate must survive translation to a MongoDB query (the driver has to turn the bitwise test into a $bitsAllSet-style filter), which is exactly the kind of real-world friction that lets a wrong-but-translatable == ship where a right-but-awkward & would not. The lesson generalises: when a query provider constrains your expressions, the temptation is to write the predicate the provider likes instead of the one the domain means — and only one of those returns weapons trucks when you search for one.

Exhibit two: twin guards, one of them dead

Two specimens of the same family. In Pacco.Services.Deliveries.Core/Entities/Delivery.cs, both Complete() and Fail() open with sequential guards that differ only in the status they test:

if (Status is DeliveryStatus.Failed)
{
    throw new CannotChangeDeliveryStateException(Id, Status, DeliveryStatus.Completed);
}

if (Status is DeliveryStatus.Completed)
{
    throw new CannotChangeDeliveryStateException(Id, Status, DeliveryStatus.Completed);
}

Same exception, same arguments, two blocks. The catalogue calls the duplication a Duplicate Code smell in miniature; the technique is Consolidate Conditional Expression — one guard, one condition (Status == DeliveryStatus.Failed || Status == DeliveryStatus.Completed), one throw. Trivial? Yes — until the day someone adds a Cancelled status and updates one twin of one pair in one of the two methods.

The second specimen has sharper teeth. IdentityService.SignInAsync in the Identity service checks the password twice:

var user = await _userRepository.GetAsync(command.Email);
if (user is null || !_passwordService.IsValid(user.Password, command.Password))
{
    _logger.LogError($"User with email: {command.Email} was not found.");
    throw new InvalidCredentialsException(command.Email);
}

if (!_passwordService.IsValid(user.Password, command.Password))
{
    _logger.LogError($"Invalid password for user with id: {user.Id.Value}");
    throw new InvalidCredentialsException(command.Email);
}

The second if can never fire: any invalid password already threw inside the first. The prescription is Consolidate Duplicate Conditional Fragments — but notice what the dead branch was for. It carries the accurate log message. The live branch logs “was not found” even when the user was found and simply presented a wrong password, so the service's telemetry mis-describes every failed sign-in. The dead code is not just clutter; it is the corpse of a better diagnostic, killed by a merge of two guards that was never finished. When you consolidate, consolidate the logging too — or accept, deliberately and in one place, that credential failures should be indistinguishable in logs.

Exhibit three: the mapper that returns null

Every aggregate-style service in the estate translates domain events to integration events through an EventMapper — a type switch. Deliveries' version nests a second switch over status:

// Pacco.Services.Deliveries.Infrastructure/Services/EventMapper.cs (trimmed)
public IEvent Map(IDomainEvent @event)
{
    switch (@event)
    {
        case DeliveryStateChanged e:
            switch (e.Delivery.Status)
            {
                case DeliveryStatus.InProgress: return new DeliveryStarted(e.Delivery.Id, e.Delivery.OrderId);
                case DeliveryStatus.Completed:  return new DeliveryCompleted(e.Delivery.Id, e.Delivery.OrderId);
                case DeliveryStatus.Failed:     return new DeliveryFailed(e.Delivery.Id, e.Delivery.OrderId, e.Delivery.Notes);
            }
            break;
        case DeliveryRegistrationAdded e:
            return new RegistrationAddedToDelivery(e.Delivery.Id, e.Delivery.OrderId, e.Registration.Description);
    }

    return null;
}

An unmapped event falls through to return null. And the copied MessageBroker that publishes the mapper's output contains, by design, a null-skip: if (@event is null) continue;. Compose the two and you get the estate's quietest failure mode: add a new domain event, forget the mapper arm, and the event vanishes at publish time with no exception, no log above trace level, and no test to notice — the aggregate raised a fact and the world never heard it. This is the Switch Statements smell from the OO-abusers family, and the by-the-book cure is Replace Conditional with Polymorphism — let each domain event describe its own integration projection. In a small service I would settle for the cheaper half-measure: keep the table, make it exhaustive, and make the default arm throw. Either way, the rule is the same: a mapper that can return null converts every future domain event into a silent no-op; make the unmapped path loud.

Exhibits four, five, six: small dead things

Three short specimens round out the tray.

The constructor that forgets its own argument. Pacco.Services.Pricing.Api/Core/Entities/Customer.cs:

public Customer(Guid id, bool isVip, int completedOrdersNumber)
{
    IsVip = isVip;
    CompletedOrdersNumber = completedOrdersNumber;
}

id is accepted and never assigned; Id stays Guid.Empty for the life of the object. Harmless today — nothing in Pricing reads Customer.Id — which is exactly why it is dangerous: it is a Dead Code specimen shaped like a working feature, waiting for the first caller who trusts the property. The catalogue offers Remove Parameter for genuinely unused parameters, but here the honest fix is the missing assignment: the parameter documents the intent, the body dropped it.

The constructor with a side effect. OrderMaker's saga assigns a fresh, anonymous CorrelationContext to an ambient accessor in its constructor — a DI-resolved object mutating shared state at construction time, on every resolve. Part 9 covered what it breaks; the smell-audit note is simply the principle: constructors acquire, they do not act. The prescription is to move the mutation into the handler that needs it, where it runs once, on purpose, with context.

dynamic on the write path. Availability's EventProcessor dispatches domain events like this:

var handlerType = typeof(IDomainEventHandler<>).MakeGenericType(eventType);
dynamic handlers = scope.ServiceProvider.GetServices(handlerType);
foreach (var handler in handlers)
{
    await handler.HandleAsync((dynamic) @event);
}

Two dynamic casts put DLR call sites inside every command that raises an event. At sample scale the performance cost is noise; the real price is that the compiler has been dismissed from the room — a handler with a mismatched signature becomes a runtime binder failure on the write path instead of a red squiggle. The cure is the standard reflection-cached typed dispatch (build and cache a delegate per event type), which is precisely the kind of glue a shared kernel would own — a thread part 13 pulls hard.

The tray, labelled

Step back from the tray and one pattern organises it: almost every specimen lives in code that exists in several near-identical copies, or in glue that every service had to hand-roll — the smells cluster exactly where the framework's surface ends. The vehicle search bug sits in a hand-written query handler; the null-skip pact between mapper and broker is copied into seven repositories; the dead guard survives because no repo's CI would ever have caught it. Polished surface, drifting underneath — the estate in one sentence.

And the boilerplate nobody reads hides one more genre of debt this audit has only brushed: the registrations. Next, an inventory of everything the estate wires up and never uses — the graveyard of registered dependencies.