Module audit

Applies when: A service function computes something only from one object's own fields.

Rule. Put invariants and calculations over one object on that object, and move logic to a service only when it spans objects or needs an injected collaborator.

Why. An anemic model scatters the rules about an object across services, and every caller needs to know where a rule lives.

def order_total(order): return sum(l.price * l.qty for l in order.lines)

class Order:
    @property
    def total(self): return sum(l.price * l.qty for l in self.lines)

Counterpoint. "Data classes should stay dumb." That is a style choice, not a free pass. If the calculation needs I/O or other aggregates, a service is right. The rule targets only functions that read nothing but one object.

Applies when: Business code reads environment variables or holds configuration values.

Rule. Read configuration in one settings module and pass it in; business code must not read the environment or hold knobs.

Why. Scattered reads hide what the app depends on and make tests mutate the process environment.

timeout = float(os.environ.get("TIMEOUT", "5"))   # inside a service

settings.request_timeout_s    # defined once in settings.py, injected

Counterpoint. "One more env read is harmless." Each one hides a deployment dependency from review and from the docs. One module is also where you add validation.

Sources: www.uber.com, tech.cabify.com

Applies when: A module or package is large, mixes concepts, or holds a very large literal.

Rule. Keep one cohesive concept per module.

Why. Banner comments and # ---- section ---- markers show a module doing several jobs. Large literals bury the logic around them. Split an oversized package into subpackages, move big literals to their own module, and inline a folder that holds one module.

service.py (1800 lines): prompts, parsing, billing, formatting; with # ==== banners
pkg/only_one.py   # a folder for a single module

service.py (logic), prompts.py (the big literal), billing.py ...
only_one.py       # folder inlined

Counterpoint. "Splitting scatters related code." True when the split is by layer or time rather than by concept. Ousterhout warns about temporal decomposition. Split by owned knowledge, not by when things run.

Sources: dev.to, shopify.engineering, tech.cabify.com

Applies when: A new class or module is introduced, or an existing interface grows.

Rule. Prefer a small interface that hides real work, and flag modules whose interface is nearly as complex as their implementation.

Why. A shallow module charges the reader the cost of learning an interface and returns nothing. The complexity moves; it does not shrink.

class Reader:
    def open(self); def read_line(self); def set_encoding(self, e); def close(self)
    # caller must sequence all four; same work in a function would be one call

def read_lines(path, encoding="utf-8") -> list[str]: ...

Counterpoint. Ousterhout and Martin disagree. Martin favours many very small functions, and Ousterhout says that yields shallow, entangled pieces. Evidence is thin on both sides. Ask the local question: does the split help a reader here?

Sources: dev.to, github.com

Applies when: A reviewer sees long methods, feature envy, data clumps, primitive obsession, speculative generality, or divergent change.

Rule. Name the smell, point to the line, and suggest the refactoring; do not file smells as taste.

Why. Smells are cheap to see and expensive to ignore, but a vague "this feels off" gives the author nothing to fix.

review: "This is messy."

review: "should-fix: price_total reads only Order fields (feature envy). Move it to Order."

Counterpoint. "Smells are subjective." Yes: they signal; they do not prove. That is why the comment names the smell, the line, and the refactoring. "This is messy" gives the author nothing to act on.

Sources: dev.to, google.github.io

Applies when: A class uses a database, HTTP, cloud or clock collaborator.

Rule. Inject boundary collaborators through the constructor; do not construct them inside or reach for a global.

Why. Hard-wired collaborators force tests to patch module internals and tie the class to one environment.

class Billing:
    def charge(self, o): return StripeClient(KEY).charge(o)

class Billing:
    def __init__(self, gateway): self._gateway = gateway

Counterpoint. "Injection everywhere is enterprise bloat." Inject only the boundary (DB, HTTP, cloud, clock). Internal collaborators stay plain and real in tests.

Applies when: A module has an intended public surface and internal files or symbols.

Rule. Make a module's internals unimportable from outside it, and have a check fail the build when a caller reaches in.

Why. Anything a caller can import becomes the interface. A naming convention such as a leading underscore stops no one, and the module can no longer change its internals without breaking callers.

from billing.internal.tax_tables import RATES   # nothing stops this

billing/__init__.py exports calculate_tax; import-linter contract forbids billing.internal.* from outside billing

Counterpoint. "A boundary check is overhead for a small repo." One lint contract is cheap, and the first outside import of an internal costs more. For a single-file module, there is no inside to protect, so mark N/A.

Tool gate: Import-boundary lint (import-linter, ESLint no-restricted-imports, package exports).

Sources: github.com

Applies when: The codebase has separable transport, logic, and outbound-boundary code.

Rule. Make dependencies point inward: surface, then logic, then boundary.

Why. Once transport types leak into logic, you cannot reuse or test the logic without the transport, and a change to the API shape ripples into the rules. Name the layers for what they do (surface, logic, boundary).

service/orders.py:  def create(request: HttpRequest) -> HttpResponse
controller/orders.py: calls repository directly

controller -> service -> repository      (imports only along this arrow)
service takes plain domain types

Counterpoint. "Three layers is ceremony for a CRUD app." For a ten-endpoint app, yes; fold them. The check applies once you have real logic. It asks only that dependencies not point outward, not that you build all three folders.

Tool gate: import-linter or similar

Applies when: A test uses mocks, patches or fakes.

Rule. Mock only at the process boundary (HTTP, DB, cloud, clock, I/O).

Why. Mocks of internals pin the implementation, so any refactor breaks tests that prove nothing. A fake looser than the real client passes tests that the real backend rejects. Keep internal objects real, make a fake accept only what the real client accepts, and assert whole outcomes. See chg-agent-mock-asserting-tests for tests that must fail when behaviour breaks.

mailer.send.assert_called_once()        # proves a call, not what was sent
class FakeStore:  def get(self, **_): ...   # accepts anything

assert outbox.sent == [Email(to="ana@example.com", template="welcome")]
class FakeStore:  def get(self, key: str): ...   # real signature

Counterpoint. "Isolated unit tests need every collaborator mocked." That tests the wiring you wrote, not the behaviour. Prefer a stub when behaviour is simple, and mock only what crosses the process edge.

Applies when: A module does anything at import or load time other than defining names.

Rule. Keep I/O at the edges: importing a package must not touch the network, read files, or read the environment.

Why. Import-time effects make tests slow and order-dependent, break tooling that merely imports the module, and fail in places that had no reason to need a network.

CLIENT = HttpClient(os.environ["API_URL"])   # at import
DATA = json.load(open("data.json"))

def make_client(settings): return HttpClient(settings.api_url)
def load_data(path): ...

Counterpoint. "Loading once at import is fast." Load once at startup, explicitly, where the caller controls it and tests can substitute it.

Applies when: A constructor does work beyond assigning its arguments.

Rule. Assign only in constructors, and put I/O, parsing, and heavy computation in a named factory.

Why. A constructor with side effects makes every instantiation, including in tests, read a file or call an API. You need a real resource or a patch just to build the object.

class PriceList:
    def __init__(self, path):
        self._prices = json.loads(path.read_text())

class PriceList:
    def __init__(self, prices): self._prices = prices
    @classmethod
    def from_file(cls, path): ...

Counterpoint. "A factory per class is ceremony." For a value object with no I/O, there is nothing to factor out. The rule targets constructors that read, call, or parse. With the factory, the test builds the object in one line.

Applies when: A module defines a variable that changes after load: a list, dict, counter, cache, client or singleton.

Rule. Keep no mutable state at module level.

Why. Hidden global state leaks between tests, between requests, and between threads, and its order of mutation is unreadable. A frozen, load-once constant such as settings is the exception. A singleton used to dodge injection is not.

_cache = {}
def get(k):
    _cache.setdefault(k, load(k))

class Loader:
    def __init__(self, cache): self._cache = cache

Counterpoint. "A module global is the simplest cache." It is also unbounded and shared by everything. If you need one, bound it (LRU or TTL), inject it, and tie its lifetime to what it caches.

Applies when: The diff adds or grows a module named utils, helpers, common, or misc.

Rule. Put a helper in the module that owns the concept, or in a new module named after the concept.

Why. A grab-bag has no owner and no cohesion. It becomes the dependency of everything and the place where unrelated code accretes.

utils.py: parse_date, send_email, price_with_tax, retry

dates.py, notifications.py, pricing.py, retry.py

Counterpoint. "Tiny helpers do not deserve a file." Then inline them where they are used (YAGNI applies). When one is shared, naming its concept is the work that makes it findable.

Sources: dev.to

Applies when: A module exposes a public function, class or re-export.

Rule. Expose domain types in a module's public interface, and keep transport, storage and framework types behind it.

Why. A public signature that returns an ORM row or a protobuf message ties every caller to that library and schema. A storage change then ripples through code that never cared about storage.

def get_user(id: str) -> UserRow: ...          # SQLAlchemy model
from proto.user_pb2 import User                # re-exported as the API

def get_user(id: UserId) -> User: ...          # plain domain dataclass; mapping lives inside

Counterpoint. "Mapping costs a copy and a class." For a thin pass-through with no second consumer, that cost can exceed the benefit; say so in the review. The rule bites when the type crosses a package or team boundary.

Sources: github.com

Applies when: A shared helper or base class has gained flags or conditionals so different callers can bend it.

Rule. Inline an abstraction that has grown flags for its callers, then re-split it.

Why. Extract an abstraction when real call sites need it (mod-yagni). A wrong one is harder to remove than the duplication it replaced, because each new caller adds a branch until nobody can say what the code is for.

def render(doc, *, as_pdf=False, for_email=False, legacy=False, skip_header=False): ...

render_pdf(doc), render_email(doc)   # small, similar, independent

Counterpoint. Sources disagree. Ousterhout lists repetition as a red flag (one fact in many places), and the AHA/Sandi Metz line prefers duplication to a wrong abstraction. They fit together: dedupe knowledge (a rule, a format, a constant), wait on shape, and extract only when the second real caller shows what is shared.

Sources: sandimetz.com, kentcdodds.com, dev.to

Applies when: The change introduces or reuses names for domain concepts.

Rule. Use one name for one concept across the codebase.

Why. If customer, client, and account all mean the same thing, readers wonder whether they differ. A name that misdescribes behaviour is worse than a vague one. Follow the project glossary, and never choose a name that says something the code does not do.

get_customer(); load_client(); fetch_account()   # same entity
def profile_data(df): ...   # inspects column types, not performance

get_customer(); load_customer(); fetch_customer()
def discover_structure(df): ...

Counterpoint. "Names are bikeshedding." Reviewers should not argue taste. They should check that the name is consistent and true. Vague names (names that could mean many things) are a design smell in Ousterhout's list. Do not rename wholesale in a feature change; make the rename a separate change.

Sources: google.github.io, dev.to

Applies when: A module wraps protocol, parsing or business logic and offers it to callers that may be sync or async.

Rule. Keep logic in plain synchronous functions with no I/O, and add async wrappers at the edge only.

Why. An async-only API forces every caller into an event loop, doubles testing, and makes accepting work so cheap that overload goes unnoticed. A synchronous core is tested without a loop and serves both worlds.

async def parse_frame(reader):          # parsing and socket reads fused
    header = await reader.read(4)
    ...

def parse_frame(buf: bytes) -> Frame | None: ...   # pure, sync
async def read_frame(reader):                        # thin I/O shell
    return parse_frame(await reader.read(4))

Counterpoint. "Everything here is async anyway." That is fine for the shell. The test is whether the logic needs I/O to run. If it does not, it should not need await either.

Sources: sans-io.readthedocs.io, lucumr.pocoo.org

Applies when: A .proto file defines an enum.

Rule. Give every protobuf enum a zero value named <ENUM>_UNSPECIFIED with no real meaning.

Why. A reader that sees an unset or unknown field gets zero. If zero means a real state, such as PAID, a missing field reads as a payment.

enum Status {
  PAID = 0;
  REFUNDED = 1;
}

enum Status {
  STATUS_UNSPECIFIED = 0;
  PAID = 1;
  REFUNDED = 2;
}

Counterpoint. "Zero as the common default saves a field on the wire." The saving is a byte on a field already at its most common value, and the cost is a silent wrong state. Published enums keep their numbers, so the fix for an existing enum is a new unspecified name only if no value has to be renumbered.

Tool gate: Protobuf linter (buf lint, ENUM_ZERO_VALUE_SUFFIX).

Sources: protobuf.dev, dropbox.tech

Applies when: The code adds a subclass or an implementation of an interface.

Rule. Make a subtype pass its parent's tests: accept every input the parent accepts, promise at least what it promises, and keep its invariants.

Why. Callers hold the parent type. A subtype may weaken preconditions and strengthen postconditions, never the reverse, and must keep the parent's invariants. The parent's black-box tests are the cheapest way to check that. A subclass that rejects valid input or weakens a result breaks them, and the failure appears far from the subclass.

class ReadOnlyFile(File):
    def write(self, data):
        raise NotImplementedError      # parent accepts any bytes

class ReadOnlyFile(Readable): ...      # separate type; File(Readable, Writable)
# or: the parent's black-box tests run unchanged against ReadOnlyFile

Counterpoint. "Inheritance is for sharing code." Then share it by composition. A subclass that ignores most of what it inherits (mod-fowler-smells) is a sign that the hierarchy is wrong.

Tool gate: Run the parent's test suite against every subclass; mypy flags incompatible overrides.

Sources: www.cs.columbia.edu, www.eiffel.org, mypy.readthedocs.io, www.hillelwayne.com

Applies when: The application has a main module, an entry point, or a bootstrap file.

Rule. Keep the entry module to wiring (bootstrap, configuration, lifespan) and put no business logic in it.

Why. Logic in the entry module cannot be imported by tests or other entry points without starting the app.

main.py: defines routes, validates, computes prices, starts server

main.py: build settings, wire dependencies, mount routers, start

Counterpoint. "It is only 20 lines." It will not stay 20. The rule gives the next feature an obvious home that is not main.

Applies when: Input checks appear in more than one layer, or hand-rolled checks appear on a dict.

Rule. Validate at the boundary, once, with a schema or model, so that inner layers receive typed, already-valid data.

Why. The same check copied into three layers drifts until nobody knows which one is authoritative.

def create_order(body: dict):
    if not body.get("items"): raise ...
    if body.get("currency") not in {"EUR", "USD"}: raise ...

class CreateOrder(Schema):
    items: list[Item]  # min 1
    currency: Currency # enum, unknown fails at parse

Counterpoint. "Defence in depth means validating everywhere." At trust boundaries, yes: every external input. Inside, repeated validation hides who owns the rule. Use types to carry "already valid" inward.

Sources: shopify.engineering, netflixtechblog.com, www.uber.com, blog.cloudflare.com