Skip to main content
Back to Library
the readable codebase

The Pull Request That Passed Every Check and Shipped a Design Problem

5 min read Chapter 23 of 27

The Pull Request That Passed Every Check and Shipped a Design Problem

The Smell

A developer on the logistics platform team submits a pull request titled “Add customs declaration support.” The CI pipeline is green. Checkstyle passes. SonarQube reports no new issues above threshold. ArchUnit tests pass. All 847 unit tests pass. The pull request adds 340 lines across 4 files. Two reviewers approve within an hour.

The change adds a handleCustomsDeclaration method to ShipmentService. The method is 45 lines with a cognitive complexity of 12, under the threshold of 15. It takes a ShipmentContext parameter and adds customs-specific fields to it. It calls CarrierService directly for carrier-specific customs requirements.

Every automated check is satisfied. The code ships. Three weeks later, the team discovers the problems:

  1. ShipmentService gained yet another responsibility, growing from 3,200 to 3,400 lines
  2. The ShipmentContext class gained three more fields, growing from 12 to 15
  3. A direct dependency from ShipmentService to CarrierService bypasses the CarrierGateway that was supposed to be the public API
  4. The method name handleCustomsDeclaration follows the handle* pattern that Chapter 4 identified as a naming smell

None of these problems trigger automated checks. All of them increase cognitive load for the next developer who works in this area.

The Cognitive Cost

A review that catches only formatting and syntax serves the same function as a linter. A review that catches design drift serves a function no tool can replicate. The cognitive cost of missing a design problem in review is not the cost of the single change. It is the cost of every future change that builds on the wrong foundation.

The customs declaration method added to ShipmentService tells the next developer: “It is acceptable to add methods to ShipmentService.” The ShipmentContext expansion tells the next developer: “It is acceptable to add fields to ShipmentContext.” The direct dependency on CarrierService tells the next developer: “It is acceptable to bypass CarrierGateway.” Each precedent makes the next violation cheaper to commit.

The Before

The pull request as originally submitted:

// HARD TO READ: This change passed all automated checks.
// It adds customs responsibility to ShipmentService (already 3,200 lines).
// It expands ShipmentContext (already 12 fields).
// It bypasses CarrierGateway by calling CarrierService directly.

// In ShipmentService.java (adding to a 3,200-line class)
public void handleCustomsDeclaration(ShipmentContext context) {
    if (!context.getShipment().isInternational()) {
        return;
    }

    // Direct call to CarrierService, bypassing CarrierGateway
    CarrierCustomsRequirements requirements =
        carrierService.getCustomsRequirements(context.getCarrierId());

    CustomsDeclaration declaration = new CustomsDeclaration();
    declaration.setShipmentId(context.getShipmentId());
    declaration.setHarmonizedCodes(
        extractHarmonizedCodes(context.getShipment().getItems()));
    declaration.setDeclaredValue(
        calculateDeclaredValue(context.getShipment().getItems()));
    declaration.setOriginCountry(context.getShipment().getOrigin().getCountry());

    if (requirements.requiresCommercialInvoice()) {
        declaration.setCommercialInvoice(
            generateCommercialInvoice(context));
    }

    context.setCustomsDeclaration(declaration);       // Expanding the context
    context.setCustomsStatus("PENDING");              // Expanding the context
    context.setRequiresCustomsClearance(true);         // Expanding the context

    customsDeclarationRepository.save(declaration);
}

A review that approves this without comment has endorsed:

  • Growing the God class
  • Growing the data bag
  • Violating the module boundary
  • Using a vague method name

The Fix

The review catches the design problems. Here is what useful review comments look like, and what noise looks like:

// USEFUL review comment:
// "This adds a new responsibility to ShipmentService, which already has 47 methods.
//  Can this be a separate CustomsDeclarationService? The customs logic doesn't
//  share dependencies with rate calculation or tracking."
//
// This comment identifies the God class growth, suggests a concrete alternative,
// and provides a reason (no shared dependencies).

// NOISE review comment:
// "Maybe rename 'requirements' to 'customsRequirements' for clarity?"
//
// This is a style preference, not a design observation.
// It does not identify the structural problem.

// USEFUL review comment:
// "This calls CarrierService directly, but we established CarrierGateway as the
//  public API in the carrier package. The ArchUnit test doesn't catch this because
//  both are in the service layer. Can this go through CarrierGateway.getCustomsRequirements()?"
//
// This identifies a boundary violation that automated tools missed.

// NOISE review comment:
// "Nit: extra blank line on line 34."
//
// This is what Checkstyle is for. If the formatter didn't catch it,
// configure the formatter. Don't spend review time on whitespace.

// USEFUL review comment:
// "ShipmentContext is gaining three more fields. It's now at 15 fields,
//  and this method only uses 4 of them. Would a CustomsDeclarationRequest
//  record with just the fields this method needs be cleaner?"
//
// This identifies the data bag growth and suggests a typed alternative.

The same pull request, revised after review:

// READABLE: Customs declaration is a separate service.
// It declares its own dependencies. It uses typed parameters.
// It goes through CarrierGateway for carrier-specific requirements.

package com.logitrack.shipment.customs;

public class CustomsDeclarationService {

    private final CarrierGateway carrierGateway;
    private final CustomsDeclarationRepository declarations;

    public CustomsDeclaration createDeclaration(Shipment shipment) {
        if (!shipment.route().isInternational()) {
            return CustomsDeclaration.notRequired(shipment.id());
        }

        CarrierCustomsRequirements requirements =
            carrierGateway.getCustomsRequirements(shipment.carrierId());

        CustomsDeclaration declaration = CustomsDeclaration.create(
            shipment.id(),
            shipment.cargo().harmonizedCodes(),
            shipment.cargo().declaredValue(),
            shipment.route().originCountry(),
            requirements
        );

        return declarations.save(declaration);
    }
}

The revised version:

  • Lives in its own class with 2 dependencies instead of adding to a class with 23
  • Uses CarrierGateway, the public API, instead of bypassing it
  • Takes a Shipment parameter instead of the 15-field ShipmentContext
  • Returns a value instead of mutating a shared context object
  • Has a class name (CustomsDeclarationService) that makes its responsibility obvious

The Rule

Before approving a pull request, check four things that automation cannot: (1) Does this change add responsibility to a class that is already too large? (2) Does it bypass an established module boundary? (3) Does it grow a data bag instead of using typed parameters? (4) Does the method name describe a single specific action? If any answer is yes, leave a comment that identifies the structural problem, explains the cognitive load it creates, and suggests a concrete alternative. Do not approve to avoid conflict. The cost of the conflict is one uncomfortable conversation. The cost of the approval is permanent design debt.