skip to content

Anti-Patterns Reviewers Reject

4 roadmaps13 questions

This branch groups by what review rejects rather than by the scale of the decision: an interface invented before a second implementation, package state built in init, and a dropped error.

part ofGo (Golang)overview, primer and where to startread it →
on this pageshow

explore

questions

13

In Go, why do reviewers reject an exported interface declared beside its only implementation?

level: juniorimportance: must knowfreq 72%

answer

  1. one implementation is not a choice
  2. who needs the abstraction, you or the caller?
  3. no implements keyword, so add it later
  4. accept interfaces, return structs

basics

~20 s

Go checks interface satisfaction implicitly, so whoever needs the abstraction can declare it later. An interface written beside its only implementation adds indirection now, hides the concrete type's other methods and fields, and buys nothing.

solid answer

~40 s

Because in Go the abstraction is free to add later. A type satisfies an interface just by having the methods, so a package that needs a seam declares its own one- or two-method interface and the producer never has to know. Declaring `type Authorizer interface` next to the single `authorizer` struct that implements it inverts that: the producer guesses which methods matter, every importer couples to that guess, the constructor's documentation degrades into a bare method list, and callers lose any method or field the interface omits. It also freezes evolution -- adding a method to an exported interface breaks anyone who implemented it, while adding a method to a struct breaks nobody. The guideline is "accept interfaces, return structs": take a narrow interface where you consume behaviour, and return the concrete `*Client`.

code

go · 12 lines
go
// package payments
type Authorizer interface {
	Authorize(ctx context.Context, cardID string, cents int64) (string, error)
}

type authorizer struct{ db *sql.DB }

func New(db *sql.DB) Authorizer { return &authorizer{db: db} }

func (a *authorizer) Authorize(ctx context.Context, cardID string, cents int64) (string, error) {
	// ... insert the authorisation row, return its id
}

go deeper

for a junior

Be ready to state the guideline and its reason: accept interfaces, return structs, because Go satisfies interfaces implicitly and the abstraction can be added later by whoever needs it.

for a middle

Explain the mechanics behind the guideline: satisfaction is checked at the use site, so the interface can live in the consumer; and adding a method to an exported interface breaks implementers while adding one to a struct does not.

for a senior

Show the review judgment. Name the concrete costs to importers, name the cases where exporting an interface is genuinely correct, and describe how you would remove one that already exists without stranding callers.

for a principal

Own the convention. Decide whether the codebase declares interfaces on the producer side at all, write down the exceptions, and be able to defend the choice against a team that wants uniform doubles everywhere.

## The shape reviewers flag It is a package that contains all three of these at once: ```go type Authorizer interface { Authorize(ctx context.Context, cardID string, cents int64) (string, error) } type authorizer struct{ db *sql.DB } func New(db *sql.DB) Authorizer { return &authorizer{db: db} } ``` One interface, one implementation, and a constructor that hands back the interface. In a language where a type must name the interfaces it implements, this is unavoidable: if you do not declare the interface up front you cannot introduce it later without editing the implementing type. Go removed that constraint, and the habit did not travel well. ## Why Go makes it unnecessary Go has no `implements` keyword. A type satisfies an interface if its method set contains the interface's methods, and the compiler checks that at the point of use -- the assignment, the argument, the return. Nothing has to be declared ahead of time, and the implementing type does not have to import the interface at all. The practical consequence is that the abstraction can be created by whoever turns out to need it, whenever they need it, at zero cost to the package that supplies the concrete type. That is why the standard library's smallest interfaces (`io.Writer`, `io.Reader`, `fmt.Stringer`) are one method wide and sit where they are consumed: `io.Copy` needs exactly two methods, so it asks for exactly two. ## What the premature version actually costs **The producer guesses.** The package author has to predict which subset of behaviour matters to consumers. Consumers rarely need the same subset, so the exported interface is either too wide (a fourteen-method mirror of the struct, which abstracts nothing) or too narrow for the next caller. **Importers couple to the guess.** Once the interface is exported, other packages name it in their own signatures, store it in their own structs and implement it. It is now part of your compatibility surface, and it is a surface you gained nothing from. **Callers lose the type.** A constructor returning `Authorizer` throws away every method and every exported field the interface does not list. Adding a method to `*Client` later does not reach those callers -- they hold an interface value and need a type assertion to get at it. Documentation suffers the same way: godoc for the constructor points at a list of method signatures instead of the real type with its examples and field comments. **Evolution becomes a breaking change.** Go interfaces have no default method bodies. Adding a method to an interface other packages implement stops their types from satisfying it, and they stop compiling. Adding a method to a struct breaks nobody. Exporting the interface converts a safe change into an unsafe one. **The indirection is real but invisible.** Calls through an interface are indirect and cannot be inlined the way a direct call on a concrete type can. That rarely decides anything on its own, but it is a cost paid for an abstraction with one implementation. ## Where the interface belongs instead In the package that consumes the behaviour, as narrow as that package's actual use: ```go // package checkout type authorizer interface { Authorize(ctx context.Context, cardID string, cents int64) (string, error) } func Charge(ctx context.Context, a authorizer, cardID string, cents int64) error { _, err := a.Authorize(ctx, cardID, cents) return err } ``` The payments package still returns `*Client`, and `*Client` satisfies `checkout.authorizer` without either package knowing about the other's declaration. The interface can even be unexported -- callers pass a concrete value in, and only `checkout` needs the name. ## The exceptions, which are real - **Two or more implementations ship today.** A package that offers an in-memory and a durable variant of the same behaviour has an actual set to abstract over. - **The interface is the package's product.** `sort.Interface`, `hash.Hash` and `io.Writer` exist so that *other people* implement them. That is the opposite of a premature interface: there is no single implementation, there is a contract. - **The return type genuinely varies.** A constructor that returns one of several concrete types by argument has to return an interface. - **A boundary you do not own.** A vendor SDK or a service you must stand in for is a legitimate seam -- though even then the interface is usually best declared by the package that calls it. ## What to say in review Ask who needs the abstraction. If the answer is "nobody yet" or "the tests", delete the interface, return `*Client`, and let the consumer declare a narrow interface when it has a reason to. Nothing is lost by waiting, because implicit satisfaction means the abstraction can be introduced later without touching the implementation.

  • When is it right for a package to export an interface for its own type?
    When more than one implementation ships today, when the interface is the product rather than the plumbing -- `sort.Interface`, `hash.Hash` and `io.Writer` exist so other packages implement them -- or when a constructor genuinely returns one of several concrete types depending on its arguments. One implementation and a hope is not a reason.
  • What does a caller gain when the constructor returns *Client instead of an interface?
    Every exported method and field, real documentation on the type, and the ability to receive new methods without a type assertion or a source change. The caller can still take a narrow interface in its own function signatures, so nothing about testability is lost by returning the concrete type.
  • Does returning a concrete type stop a consumer from substituting a stand-in?
    No. Satisfaction is implicit, so the consuming package declares the one- or two-method interface it needs and anything with those methods -- the real `*Client` or a small struct written for a test -- fits it. The producer does not participate in that decision at all.

Do not hand someone a keyhole when you can hand them the key; if they ever want a keyhole, Go lets them cut their own without asking you.

saying these in an interview costs you the question

  • Every struct should have a matching interface
  • The interface is needed so the package can be tested
  • Go requires the interface before a type can implement it
  • Returning an interface hides the implementation, so it is safer
  • The interface makes it easy to swap the database later
open as a page

Why does Go compile a call whose returned error you never check?

level: juniorimportance: must knowfreq 72%

basics

~20 s

Go's compiler rejects unused variables and unused imports, but not unused return values. A call like f.Close() is a complete statement on its own, so nothing forces you to look at the error it returns.

open as a page

What is wrong with a package-level `var apiURL = os.Getenv("API_URL")` in a Go package?

level: juniorimportance: must knowfreq 68%

basics

~20 s

It runs at package initialization, before main, so nothing can supply or check the value: the package cannot report a missing setting as an error, and a test can only change it by writing to a global.

open as a page

Why is `defer f.Close()` on a file you just wrote an error-swallowing bug?

level: middleimportance: must knowfreq 58%

basics

~20 s

A deferred call's return value is thrown away, so defer f.Close() discards the error reporting a failed write. Closing the file also does not flush a bufio.Writer above it, so buffered bytes are never written and the run still reports success.

open as a page

What does an exported Go function lose by taking a parameter of type any?

level: middleimportance: should knowfreq 45%

basics

~20 s

An any parameter gives up the compile-time contract. The signature no longer says what is accepted, so the rule lives in a doc comment and a runtime type switch, and a caller's wrong type fails in production.

open as a page

In a Go CSV loop, what does `if err != nil { continue }` hide from the caller?

level: middleimportance: should knowfreq 44%

basics

~20 s

It converts a failure into an absence. Bad records vanish, the loop finishes normally, and the function returns success, so the caller cannot tell a clean run from one that skipped a quarter of the input.

open as a page

What does it cost when a Go CLI's subcommands register themselves into a package-level map from init?

level: middleimportance: should knowfreq 38%

basics

~20 s

The set of subcommands becomes a property of the import graph rather than of readable code, registration cannot return an error so conflicts only panic before main, and every test shares one registry it cannot rebuild.

open as a page

Your payments package exports a 14-method interface purely so tests can fake it, the fake has drifted, and every test still passes. What do you change?

level: seniorimportance: should knowfreq 48%

basics

~20 s

Shrink the seam and verify it. An interface pins method signatures, never behaviour. Let each consumer declare the one or two methods it calls, then run one shared behaviour suite against both the stand-in and the real client.

open as a page

A Go nightly job exits 0 but its output file is short a quarter of its rows. How do you find the swallowed error?

level: seniorimportance: should knowfreq 38%

basics

~20 s

Establish the numbers first: records read, records handed to the writer, and rows actually present when you re-open and count the file. The gap says which side lost them, and a zero exit code means a returned error nobody read.

open as a page

Under `go test -shuffle=on` a package's tests fail, though each passes alone. How do you find the cause?

level: seniorimportance: should knowfreq 45%

basics

~10 s

Reproduce with the seed the shuffle prints, add -count=1 to defeat the test cache, then bisect with -run. The culprit is usually a package-level variable that one test writes and never restores.

open as a page

As a Go tech lead, would you require every service package to export an interface so consumers can mock it?

level: principalimportance: should knowfreq 33%

basics

~20 s

No, not as a blanket rule. Go satisfies interfaces implicitly, so a consumer can declare its own seam whenever it needs one; the mandate buys uniformity at the price of a second permanent API surface per package.

open as a page

In Go, what breaks when you add a method to an interface your package already exports?

level: middleimportance: nice to knowfreq 38%

basics

~20 s

Every type outside your package that satisfied the old interface stops satisfying it and fails to compile. Go interfaces have no default method bodies, so widening an exported interface breaks implementers; adding a method to a struct breaks nobody.

open as a page

Your Go codebase is full of package-level mutable state and you have one migration budget. What changes first?

level: principalimportance: nice to knowfreq 28%

basics

~20 s

Spend the budget on the globals that cost you daily: mutable state tests must swap, and import-time work that can fail. Bless immutable package values, write the rule for new code first, and enforce it in CI.

open as a page