Anti-patterns

The 30-second version

The key word is looks. Nobody writes a 2,400-line UserManager on purpose. Each step that got it there was reasonable: "it's about users, so it goes in UserManager." An anti-patternAnti-patternA common way of solving a problem that looks reasonable at first but, over time, causes more trouble than it solves. Unlike a plain mistake, people repeat it on purpose because it seems like a good idea. is a repeated mistake, made by smart people, because the short-term reward is real.

Smells are the symptoms

You rarely see an anti-pattern directly. You see its symptoms: a file nobody wants to open, a "small" change that touched nine files, a function with six true/false parameters. These symptoms are called code smellsCode smellA surface sign that code may have a deeper design problem — a 2,000-line file, a function with 9 parameters. Not a bug by itself, but worth a look..

A smell is a reason to look, not proof of guilt. Sometimes a long function is just a long, simple list of steps. Treat smells like a cough: worth a check, usually not an emergency.

What's in this topic

• §02–§04: the "big three" structural anti-patterns: the god object, changes that scatter (shotgun surgery), and abstractions built too early.
• §05–§08: smaller, everyday smells you'll meet in code review this week: stringly-typed values, flag arguments, code in the wrong place, leaky boundaries, global state, magic numbers, concurrency shortcuts.
• §09: the same ideas at the scale of a whole system.
• §10: practice on real code: you review, the widget grades you.
• §11–§12: fixing anti-patterns safely, and knowing when not to.

The god object

Here's one. This is what Go's documentation tool prints for a real-looking UserManager, cut short:

type UserManager struct {
	// Has unexported fields (hidden from other packages).
}

func NewUserManager(db *sql.DB, smtp *smtp.Client, pay *stripe.Client, cfg Config) *UserManager
func (m *UserManager) Login(email, password string) (*Session, error)
func (m *UserManager) ResetPassword(email string) error
func (m *UserManager) Charge(userID string, cents int64) error
func (m *UserManager) Refund(chargeID string) error
func (m *UserManager) SendWelcomeEmail(userID string) error
func (m *UserManager) SendReceipt(chargeID string) error
func (m *UserManager) UpdateName(userID, name string) error
func (m *UserManager) SetAvatar(userID string, img []byte) error
    … 30 more methods

Read the method names: login, payments, emails, profile pictures. Those are four different jobs, done for four different groups of people (security, finance, marketing, product). In JavaScript it's the same story with a class UserService or a 3,000-line utils.js.

How it forms: the gravity of the easy place

No one designs a god objectGod objectOne class, struct or module that knows and does far too much — login, billing, emails, reports — so almost every change has to go through it.. It grows, one sensible step at a time:

Each new method lands where it's easiest to add, and the biggest file is always the easiest place: it already has the database connection, the config and the helpers. So big files attract more code, like a planet pulling in rocks. That pull is why god objects are an anti-pattern and not a one-off mistake.

Why it hurts

Inside a god object, the methods share the same fields: m.db, m.cache, m.cfg. That makes them coupledCouplingHow much one piece of code has to know about another. Tightly coupled code breaks together; loosely coupled code can change independently.. A change in the login code can break billing, because both read the same m.cache. Try it: pick a feature request and see how far the damage reaches.

Same 38 methods, same 2,400 lines — but split into four modules, a change touches about a quarter of the code and a fraction of the callers. The work didn't shrink. What you have to understand and re-test before you can make it did.

How to spot one

Some signs of a god object. Two or three together are a strong signal:

• The name is vague: Manager, Service, Helper, Utils, Processor, Common. A precise name like InvoiceRenderer doesn't leave room to add login code.
• You can't describe it without "and": "it handles login and billing and emails."
• Low cohesionCohesionHow closely the things inside one module belong together. High cohesion = everything in it serves one purpose.: groups of methods that use completely different fields. The email methods never touch the payment client, and vice versa.
• Everything uses it, and it uses everything. Lots of files import it, and it imports lots of packages. The terms for this are fan-in and fan-outFan-in / fan-outFan-in: how many other files use this module. Fan-out: how many other modules this one uses. A module with both very high is a knot everything runs through..
• It's at the top of the "most changed files" list. §11 shows how to measure that with one git command.

The fix: split along axes of change

Don't split by size ("first 1,000 lines go in file A"). Split by who asks for changes. For each method, ask: if this needs to change, whose request will it be? Methods with the same answer belong together. Each answer is one axis of changeAxis of changeOne independent reason the code might need to change, usually driven by one group of people: “the billing rules changed”, “marketing wants new email wording”. Good modules have one axis each..

// auth/: changes when security rules change
type Authenticator struct {
	users    UserStore    // ← only the fields this job needs
	sessions SessionStore
}

// billing/: changes when finance rules change
type Billing struct {
	users    UserStore
	payments PaymentGateway
}

// mailer/: changes when marketing changes wording
type Mailer struct {
	smtp      SMTPClient
	templates *template.Template
}

Notice that each type now lists only the dependenciesDependencySomething your code needs to do its job — another type, a database client, a logger, a library. If A calls B, A depends on B. it really uses. Mailer can't accidentally reach into payments: it doesn't have a payments client. (UserStore, PaymentGateway and friends are small interfacesInterfaceA list of method names (and their signatures) with no code. Any type that has those methods can be used wherever the interface is expected. In Go this happens automatically, without writing “implements”.. Dependency injection covers how to wire them up.)

This idea, "one module, one reason to change", is the Single Responsibility PrincipleSingle Responsibility Principle (SRP)A module should have one reason to change — it should serve one group of people (finance, marketing, security…), not several. The S in SOLID., the S in SOLID.

Shotgun surgery vs divergent change

§02 judged code by what it looks like. A sharper test is how it behaves when you change it, because changing code is what costs time. There are two opposite ways it goes wrong.

"Add a currency to orders" edited:
  order.go         orders.sql        OrderForm.jsx
  OrderList.jsx    receipt_email.go  invoice_pdf.go
  sales_report.go  csv_export.go     validate.go
→ 9 files for one idea

user.go was edited for:
  "Add 2-factor login"         "Change the tax rule"
  "New welcome email wording"  "Avatar upload"
  "GDPR data export"           "Fix password reset"
→ 6 unrelated reasons, 1 file

• Shotgun surgeryShotgun surgeryA smell where one small change forces you to edit many files at once — like a shotgun blast hitting many spots — because one idea is scattered across the codebase.: one idea, such as "how an order's money is formatted and rounded", has been copied into many places. Each change is like a shotgun blast: many small edits in many files. Forget one and you ship a bug: the receipt says USD while the PDF says EUR.
• Divergent changeDivergent changeThe opposite smell of shotgun surgery: one file keeps being changed for many unrelated reasons (billing, emails, login…), because it holds several ideas at once.: one file holds many ideas, so it changes for many unrelated reasons. This is the god object from §02, seen through its history. Four teams edit the same file, so they step on each other's changes.

See both in a heatmap

Here each row is a change someone made and each column is a file. Shotgun surgery shows up as long rows; divergent change shows up as a tall column. Healthy code looks like a rough diagonal.

The fixes are opposites, too

Both fixes aim at the same goal from opposite sides. Things that change together should live together, and things that change for different reasons should live apart. The textbook names for this are high cohesionCohesionHow closely the things inside one module belong together. High cohesion = everything in it serves one purpose. and low couplingCouplingHow much one piece of code has to know about another. Tightly coupled code breaks together; loosely coupled code can change independently..

Premature abstraction

The god object is too little structure. This section is about the opposite mistake: too much structure, too early. It's the anti-pattern that careful, well-read developers fall into, because it looks like good engineering.

Speculative generality: flexibility nobody asked for

Speculative generalitySpeculative generalityCode made flexible “just in case” — extra interfaces, options, plug-in points — for needs that don't exist yet and often never arrive. means building for needs you imagine might come. Here's a typical example: a team needs to send one kind of email.

type Notifier interface {
	Notify(ctx context.Context, m Message) error
}

type NotifierFactory interface {
	New(kind string) (Notifier, error)
}

type notifierRegistry struct {
	makers map[string]func() Notifier // name → function that builds a Notifier
}

// …and exactly one implementation:
type EmailNotifier struct{ smtp SMTPClient }

type Mailer struct{ smtp SMTPClient }

func (m *Mailer) SendWelcome(ctx context.Context, to string) error {
	return m.smtp.Send(ctx, to, "Welcome!", welcomeBody)
}

The left side has an interfaceInterfaceA list of method names (and their signatures) with no code. Any type that has those methods can be used wherever the interface is expected. In Go this happens automatically, without writing “implements”., a factory (a type whose only job is to create other objects) and a registry (a lookup table from a name like "email" to the code that builds it), all "in case we add SMS or Slack later". Two years on, it still has one implementation. Meanwhile, every new developer has to read through three layers to find out that it just sends an email.

Signs of speculative generality:

• An interface with one implementation and no test fakeTest double (dummy, stub, spy, mock, fake)A stand-in for a real dependency in tests — e.g. a fake payment client that records calls instead of charging a card. Dummy, stub, spy, mock and fake are the common kinds. using it.
• Parameters or options that no caller passes.
• A generic Repository[T] or plugin system with exactly one user. ([T] is a Go type parameterType parameter (generics)A placeholder for a type, written in square brackets in Go: func Max[T cmp.Ordered](a, b T) T works for ints, floats or strings. The compiler fills in T at each call. Added in Go 1.18..)
• Comments like "for future use" or "in case we need it".

The principle behind this is YAGNIYAGNI“You Aren't Gonna Need It”: don't build features or flexibility until a real need shows up. Predicted needs are usually wrong.: You Aren't Gonna Need It. Guesses about future needs are usually wrong, and when the real need arrives it rarely fits the shape you guessed.

Premature abstraction: merging things that only look alike

Premature abstractionPremature abstractionMerging pieces of code into one shared function or type before you know whether they really change for the same reasons. Later differences turn into flags and special cases. is the sneakier version. Two pieces of code look the same, so you merge them into one shared function. Then the two callers' needs drift apart, and every difference becomes a new option or true/false flag. Step through a real-world story:

// orders.js
function exportOrders(orders) {
  const lines = ["id,total"];
  for (const o of orders) lines.push(`${o.id},${o.total}`);
  return lines.join("\n");
}

// users.js
function exportUsers(users) {
  const lines = ["id,email"];
  for (const u of users) lines.push(`${u.id},${u.email}`);
  return lines.join("\n");
}

The trouble started at step 2. The extraction itself was neat. But it bet that "export orders" and "export users" would change for the same reasons, and they didn't. Finance owns one; the privacy team owns the other.

Duplication is far cheaper than the wrong abstraction.— Sandi Metz, “The Wrong Abstraction” (2016)

Why is the wrong abstraction more expensive than duplication?

1. Duplication is easy to remove later. Search for the copies and merge them; you only need to understand what they do today.
2. A wrong abstraction is hard to remove. Every caller depends on it, each with a different mix of flags. To untangle it you have to understand all the callers at once.
3. It invites more of the same. Once a shared function exists, the path of least resistance is "add one more flag". It feels disrespectful to rip it out.

DRY is about knowledge, not about lines that look alike

DRYDRY“Don't Repeat Yourself”: each piece of knowledge (a rule, a formula) should live in one place. It's about knowledge, not about two lines of code that happen to look alike. ("Don't Repeat Yourself") is usually the reason for premature merging. But DRY was always about knowledge: one rule should live in one place. Two blocks of code that look alike but encode different rules are not duplication. They just happen to look similar today.

Ask: if this rule changes, must both copies change? If yes, it's real duplication, so merge it. The CSV quoting rules in the final step are like that: they should never differ between exports. If the answer is "not necessarily", it's coincidence, so leave the copies apart.

Undoing a wrong abstraction

Metz's advice is counter-intuitive and it works. InlineInline (refactoring)Replace each call to a function with a copy of that function's body, then delete the function. It's the reverse of extracting a function. the abstraction back into every caller (copy the shared function's body into each place that calls it, then delete the function), delete the flags each caller doesn't use, and look at what's left. Usually one small, truly shared piece remains. Extract only that.

Primitives, parameters and flags

Primitive obsession

Primitive obsessionPrimitive obsessionUsing basic types (string, int, float) for things that have rules — money, emails, user IDs — so nothing stops you from mixing them up or passing invalid values. means using basic types like string, int and float64 for things that have rules. It's the most common smell in everyday code, because primitives are always right there.

func CancelOrder(userID string, orderID string) error { /* … */ }

CancelOrder(order.ID, user.ID) // ← arguments swapped. Compiles fine. Cancels the wrong thing.

Both parameters are strings, so the compiler can't tell them apart. The fix in Go is a named typeNamed type (Go)A new type built on an existing one, e.g. type UserID string. It stores the same data, but the compiler treats it as a different type, so you can't pass an OrderID where a UserID is expected.: a new type built on string that the compiler treats as different.

type UserID string
type OrderID string

func CancelOrder(user UserID, order OrderID) error { /* … */ }

uid := UserID("u-42")
oid := OrderID("o-7")
CancelOrder(oid, uid)
// ✗ compile error: cannot use oid (variable of string type OrderID) as UserID value

The lesson: create IDs in one place (when you parse a request or read a database row) and pass typed variables around from then on. Scattered string literals are exactly where named types can't help.

Money is not a float

Floating-point numbers like float64 and JS's number store values in binary, and most decimal amounts can't be represented exactly. In JavaScript, 0.1 + 0.2 gives 0.30000000000000004, and Go gives the same once the numbers are float64 variables. Multiply that by a million orders and the books stop balancing.

The usual fix is to store whole cents as an integer and keep the currency next to the amount. Wrap both in a value objectValue objectA small type that wraps a value with its rules, e.g. Money that always carries its currency and can only be created through a constructor that checks it. Two value objects with the same contents count as equal.: a small type that owns its own rules.

type Money struct {
	cents    int64  // ← whole cents: no rounding drift
	currency string // "USD", "EUR", …
}

func NewMoney(cents int64, currency string) Money {
	return Money{cents: cents, currency: currency}
}

func (m Money) Add(o Money) (Money, error) {
	if m.currency != o.currency { // ← the rule lives here, once
		return Money{}, fmt.Errorf("can't add %s to %s", o.currency, m.currency)
	}
	return Money{cents: m.cents + o.cents, currency: m.currency}, nil
}

func (m Money) String() string {
	sign, c := "", m.cents
	if c < 0 {
		sign, c = "-", -c
	}
	return fmt.Sprintf("%s%d.%02d %s", sign, c/100, c%100, m.currency)
}

Now Add can't mix currencies, and nobody can store 19.999999 dollars. This is also the cure for the shotgun surgery in §03: "how order money is formatted and rounded" now lives in one type instead of nine files.

The same trick works for emails. Check the value once, at the edge of your program (where outside data comes in: an HTTP request, a file, a form), and from then on the type itself is proof that it's valid. This is often summed up as "parse, don't validate".

type Email struct{ addr string }

func ParseEmail(s string) (Email, error) {
	a, err := mail.ParseAddress(s) // standard library: net/mail
	if err != nil {
		return Email{}, fmt.Errorf("invalid email %q: %w", s, err)
	}
	return Email{addr: a.Address}, nil
}
// Any function taking an Email can skip re-checking it.

Long parameter lists and data clumps

// ✗ Eight parameters, seven of them strings. Which order was it again?
func CreateUser(name, email, street, city, zip, country, plan string, trial bool) error

When values always travel together (street, city, zip, country), they're a data clumpData clumpA group of values that always travel together — street, city, zip — passed around as separate parameters instead of as one type.. Give the group a name. Replacing a long list with one struct or object is called introducing a parameter objectParameter objectA refactoring: replace a long list of parameters with one struct/object that holds them, ideally one that also owns the rules about them..

type Address struct {
	Street, City, Zip, Country string
}

type NewUser struct {
	Name    string
	Email   Email   // ← the value object from above
	Address Address // ← the clump, now named
	Plan    Plan
}

func CreateUser(ctx context.Context, u NewUser) error { /* … */ }

Flag arguments: the mystery true

What does this line do?

SendInvoice(inv, true, false)

You can't tell without opening SendInvoice. A true/false parameter that switches a function between different behaviors is a flag argumentFlag argumentA true/false parameter that makes one function do two different things, e.g. save(user, true). The call site doesn't say what true means.. It hides two problems: the call site is unreadable, and the function is secretly two functions sharing one body, which is the first step of the flag pile-up you watched in §04.

func SendInvoice(inv Invoice, reminder, pdf bool) error {
	if reminder { /* different subject, different template */ }
	if pdf { /* attach a PDF */ }
	// …
}

SendInvoice(inv, true, false)

// Different jobs → different names
func SendInvoice(inv Invoice, a Attachment) error
func SendReminder(inv Invoice, a Attachment) error

// A real setting → a named type, not a bool
type Attachment int

const (
	NoAttachment Attachment = iota
	AttachPDF
)

SendReminder(inv, AttachPDF) // reads like a sentence

In JavaScript, an options object helps the call site: sendInvoice(inv, { attach: "pdf" }) is far clearer than sendInvoice(inv, true). But if the flag picks between two different behaviors, split it into two functions, exactly as in Go.

Feature envy, nosy neighbors and leaky abstractions

These three smells are all about boundaries: where code lives, and how much one piece knows about another's insides. All three increase couplingCouplingHow much one piece of code has to know about another. Tightly coupled code breaks together; loosely coupled code can change independently., so a change in one place breaks code somewhere else.

Feature envy: a function that wishes it lived elsewhere

Feature envyFeature envyA smell where a function mostly reads and works with another object's data instead of its own — a sign the code probably belongs on that other object. is a function that spends its time working with another object's data. Look at what this function touches:

// package invoice
func Total(o order.Order) int64 {
	var sum int64
	for _, it := range o.Items {                  // Order's data
		sum += it.PriceCents * int64(it.Qty)      // Item's data
	}
	if o.Customer.VIP {                           // Order's data
		sum = sum * 90 / 100
	}
	return sum
}

Every line reads from Order. Nothing here is about invoices. If Order changes how it stores items, the invoice package breaks. The fix is to move the method to the data:

// package order
func (o Order) Total() int64 {
	var sum int64
	for _, it := range o.Items {
		sum += it.Subtotal() // ← each Item knows its own subtotal
	}
	if o.Customer.VIP {
		sum = sum * 90 / 100
	}
	return sum
}

// package invoice now just asks:
amount := o.Total()

Now the rule "how an order is totalled" lives inside Order, which can change its internals freely. This is encapsulationEncapsulationHiding a type's internal data behind its methods, so other code can't depend on (or break) the details. In Go, lower-case names are private to their package; JS has #private fields. doing its job. (For simplicity, totals here are plain int64 cents. In real code you'd use the Money type from §05.)

Inappropriate intimacy: reaching into someone else's pockets

Inappropriate intimacyInappropriate intimacyA smell where two modules reach into each other's internal details (private fields, internal structures), so neither can change without breaking the other. is when code digs through several objects to get at something deep inside:

// checkout.js knows the inside of customer,
// account AND wallet
order.customer.account.wallet.balance -= total;

// checkout.js asks the customer to do it
order.customer.charge(total);
// how charging works is the customer's business

The left line works, but it means checkout.js breaks if any of four objects reorganizes its fields. It also bypasses every rule the wallet should enforce, such as "balance can't go negative". The rule of thumb is the Law of DemeterLaw of Demeter“Only talk to your direct friends”: a method should call methods on its own fields and parameters, not reach through them (a.B().C().D()) into objects it doesn't own.: only talk to your direct friends (your own fields, your parameters, objects you created), not to their friends' friends. A chain of dots reaching through objects, like the line above, is sometimes called a train wreck.

Leaky abstractions: the details escape

An abstractionAbstractionA simpler “front” that hides details you don't need — e.g. a Storage interface with Save/Load hides whether data goes to disk, a database or memory. promises to hide details. A leaky abstractionLeaky abstractionAn abstraction that fails to hide the details it was supposed to hide, so callers still need to know what's underneath — e.g. a “storage” interface that returns SQL errors. breaks that promise. The most common leak is through errors:

type UserStore interface {
	Get(ctx context.Context, id UserID) (User, error)
}

// Somewhere in a handler:
u, err := store.Get(ctx, id)
if errors.Is(err, sql.ErrNoRows) { // ← the handler now "knows" it's SQL underneath
	http.NotFound(w, r) // w = the response we write, r = the incoming request
	return
}

The interface says "a store of users". But to handle a missing user, callers must know it's a SQL database. Swap in a cache or an HTTP service and every handler silently stops returning 404s. The fix is to translate the error at the boundary, inside the store, into an error the abstraction owns:

var ErrUserNotFound = errors.New("user not found") // ← part of the store's API

func (s *PostgresStore) Get(ctx context.Context, id UserID) (User, error) {
	var u User
	err := s.db.QueryRowContext(ctx,
		"SELECT id, name FROM users WHERE id = $1", id). // $1 = the first argument after the SQL (id)
		Scan(&u.ID, &u.Name) // copy the row's columns into u's fields (&x = "address of x")
	if errors.Is(err, sql.ErrNoRows) {
		return User{}, ErrUserNotFound // ← translate, don't leak
	}
	if err != nil {
		return User{}, fmt.Errorf("get user %s: %w", id, err)
	}
	return u, nil
}
// Callers now check errors.Is(err, store.ErrUserNotFound), whatever the storage is.

Errors aren't the only way details leak. Others:

• Performance: a list.Get(i) that's instant on an array but walks a chain of thousands of nodes on a linked list. Callers end up needing to know which one they have.
• Data shapes: an API that returns raw database rows, so renaming a column breaks the mobile app.
• Configuration: a "generic" queue client whose options are named after one vendor's settings.

Global state, magic values and stringly-typed code

Global mutable state

Global stateGlobal stateData any part of the program can read or change (a package-level variable, a module-level object). Convenient, but makes code hard to test and reason about. is data that any code anywhere can read or change: a package-level variable in Go, a module-level let in JS. It's the easiest way to share something, and that's the trap.

Here's the scariest version, in a Node server:

let currentUser = null;                       // ← one variable for the whole process
export const setUser = (u) => { currentUser = u; };
export const getUser = () => currentUser;

app.get("/orders", async (req, res) => {
  setUser(await auth(req));                   // request A: currentUser = Alice
  const orders = await db.ordersFor(getUser()); // ← await: other requests run meanwhile
  res.json({ orders, owner: getUser().name });  // may now be Bob!
});

A Node server handles many requests in the same processProcessOne running program (e.g. one Node server or one Go binary). It has its own memory and one or more threads.. While request A waits at an await, request B runs and overwrites currentUser. When A resumes, it reads Bob's user and shows Alice's orders labeled with Bob's name. In a worse version of the same code, Alice sees Bob's orders. (Why other requests run during an await is explained in Async / Await.)

The fix is boring and reliable: pass the value explicitly.

// ✗ Hidden dependency: the signature doesn't say it needs a database.
var db *sql.DB // set in main()

func GetUser(ctx context.Context, id UserID) (User, error) {
	return queryUser(ctx, db, id)
}

// ✓ The dependency is visible, replaceable in tests, and can't be swapped mid-run.
type UserService struct{ db *sql.DB }

func NewUserService(db *sql.DB) *UserService { return &UserService{db: db} }

func (s *UserService) GetUser(ctx context.Context, id UserID) (User, error) {
	return queryUser(ctx, s.db, id)
}

If the user is needed deep inside a chain of function calls and passing it through every layer is impractical, Node's built-in AsyncLocalStorage gives each request its own private copy of a value, which stays correct across awaits.

Why globals hurt, even without concurrency:

• The signature lies. GetUser(ctx, id) looks like it only needs an ID. It secretly needs main() to have set db first.
• Tests interfere with each other. One test sets the global to a fake, and another test, running in parallel or later, gets the wrong one. You can't give a function a test doubleTest double (dummy, stub, spy, mock, fake)A stand-in for a real dependency in tests — e.g. a fake payment client that records calls instead of charging a card. Dummy, stub, spy, mock and fake are the common kinds. (a fake used in tests) without changing a global.
• Anything can change it. When the value is wrong, every line in the program is a suspect.

Magic numbers and magic strings

A magic numberMagic number / magic stringA literal value like 86400 or "admin" dropped into code with no name explaining what it means or where else it must stay in sync. is a literal with no name: what is 86400? What is 3? Why "admn"?

if attempts > 3 {
	lock(user)
}
time.Sleep(86400 * time.Second)
if user.Role == "admn" { // typo: compiles, never true
	showAdminPanel()
}

const maxLoginAttempts = 3
const day = 24 * time.Hour

type Role string

const (
	RoleAdmin  Role = "admin"
	RoleMember Role = "member"
)

if attempts > maxLoginAttempts {
	lock(user)
}
time.Sleep(day)
if user.Role == RoleAdmin { // a typo in RoleAdmin is a compile error
	showAdminPanel()
}

One honest caveat: the typed constant only helps if you use it. Go still accepts user.Role == "admn", because a quoted literal converts to Role automatically (the same untyped constantUntyped constant (Go)A literal like "o-9", 3 or 0.1 that has no fixed type yet. Go keeps numeric constants exact (at least 256 bits of precision) and only converts them when they meet a typed value or variable — including your named types like UserID. rule from §05). So agree in review, or with a lint rule, that roles are only ever written as RoleAdmin/RoleMember.

Names do two jobs. They explain (maxLoginAttempts says what 3 means), and they keep copies in sync: change the constant and every use changes with it. In JavaScript, a frozen object plays the role of the constants: const Role = Object.freeze({ ADMIN: "admin", MEMBER: "member" }).

Not every literal needs a name. 0, 1, "", i + 1, and 100 in "percent = part / total * 100" are obvious. Name a value when a reader would ask "why this number?", or when it appears in more than one place.

Copy-paste programming

Copy-paste programmingCopy-paste programmingBuilding new code by copying existing blocks and tweaking them, so a bug or rule now lives in several places that must all be fixed separately. is building new code by copying an old block and tweaking it. It's fast, and the cost arrives later: a bug fixed in one copy stays alive in the other two, because nobody knew the copies existed.

This sounds like it contradicts §04 ("duplication is cheaper than the wrong abstraction"). It doesn't. §04 is about code that merely looks alike. Copy-paste becomes an anti-pattern when the copied thing is knowledge: a price rule, a validation regex, a tax rate. Those must stay identical, so they belong in one place from the start. Tools like jscpd (for many languages) can find copy-pasted blocks in a codebase.

Stringly-typed code and errors

Stringly-typedStringly typedUsing strings where a real type belongs — statuses, error kinds, options — so typos compile fine and callers have to compare text to find out what happened. code uses strings where a real type belongs. The "admn" above is one case. The most damaging case is error handling by message text:

// ✗ Breaks silently when the message changes
if err != nil && strings.Contains(err.Error(), "not found") { /* … */ }

// ✓ A sentinel error: compare identity, not text
var ErrNotFound = errors.New("not found")

if errors.Is(err, ErrNotFound) { /* … */ }

// ✓ An error type: when callers need details
type ValidationError struct{ Field string }

func (e *ValidationError) Error() string { return "invalid " + e.Field }

var ve *ValidationError
if errors.As(err, &ve) {
	fmt.Println("bad field:", ve.Field)
}

The message text is for humans reading logs, and you should feel free to improve its wording. Types and sentinel values are for code. Mixing the two means a typo fix in a message can break error handling three packages away. Go's conventions for this are covered in Go idioms.

Concurrency anti-patterns

Concurrency has its own anti-patterns, and they're nasty because they usually pass every test on your laptop and fail under real load. This section names the common ones and points to the topics that teach the cures. If goroutinesGoroutineA very lightweight task in Go, started with the go keyword. The Go runtime runs thousands of them on just a few OS threads. are new to you, read the primer in Async / Await §02 first.

1 · Sleeping instead of waiting

var cfg *Config

go func() { cfg = loadConfig() }()   // reads a file: ~20 ms on a laptop
time.Sleep(100 * time.Millisecond)   // ✗ a guess
fmt.Println(cfg.Port)                // nil pointer panic on a slow day

The sleep is a bet that loading always takes less than 100 ms. Here's that bet on two machines:

The fix is to wait for the event itself:

var cfg *Config
done := make(chan struct{})

go func() {
	cfg = loadConfig()
	close(done) // ← announce "ready"
}()

<-done // ← waits exactly as long as needed: 20 ms or 2 s
fmt.Println(cfg.Port)

(Simplest of all: if nothing else happens meanwhile, just call loadConfig() directly. A goroutine you immediately wait for buys nothing.) The same anti-pattern shows up in tests as time.Sleep(50 * time.Millisecond) before an assertion. That makes the test slow on good days and flaky (sometimes passing, sometimes failing) on a busy CI server, the shared machine that runs your tests on every push. Wait on a channel, or poll with a deadline: check the condition every few milliseconds, and fail the test if it isn't true within, say, 2 seconds.

2 · Fire and forget

Starting background work and never looking back is tempting. Sending the welcome email shouldn't slow down signup, right?

func (s *Server) handleSignup(w http.ResponseWriter, r *http.Request) {
	user, err := s.users.Create(r.Context(), parseForm(r))
	if err != nil {
		http.Error(w, "signup failed", http.StatusInternalServerError)
		return
	}
	go s.mailer.SendWelcome(r.Context(), user.Email) // ✗ fire and forget
	w.WriteHeader(http.StatusCreated)
}

What goes wrong:

1. Its context is cancelled immediately (Go). r.Context() is cancelledcontext.ContextA value passed as the first argument through Go calls that carries a deadline and a “stop now” signal. ctx.Done() is a channel you can receive from once it's time to stop. as soon as the handler returns, so the email call is told "stop" a few microseconds after it starts. It fails, and nobody sees it.
2. Errors vanish. No one checks the result. In Node ≥ 15, a rejected promise that nobody handles is an Unhandled rejectionUnhandled rejectionA promise failed and no code ever caught the error (no await in try/catch, no .catch). Node ≥ 15 crashes the process when this happens., which crashes the whole process by default.
3. Panics take down the server (Go). A panicpanic / recoverpanic is Go's crash-the-program error (like an uncaught throw). recover, called inside a deferred function, catches it — but only in the same goroutine. inside a goroutine that nothing recovers crashes the entire program, not just one request.
4. Shutdown loses work. When you deploy, the process exits and any half-sent emails disappear, because nothing was waiting for them.
5. Leaks. If the task can block forever (on a channel or a hung connection), you get a goroutine leakGoroutine leakA goroutine still running after the code that started it is done with it — usually stuck waiting on a channel nobody will use again. Each one holds memory, so leaks slowly grow until the process runs out. that grows with traffic. See Channels §08.

Give the task an owner: something that tracks it, bounds it, and sees its errors.

	s.bg.Add(1) // s.bg is a sync.WaitGroup that the server waits on at shutdown
	go func() {
		defer s.bg.Done()
		defer func() { // a panic here would otherwise crash the whole server
			if p := recover(); p != nil {
				s.log.Error("welcome email panicked", "panic", p)
			}
		}()
		// Keep the request's values but not its cancellation, and set our own limit.
		ctx, cancel := context.WithTimeout(context.WithoutCancel(r.Context()), 10*time.Second)
		defer cancel()
		if err := s.mailer.SendWelcome(ctx, user.Email); err != nil {
			s.log.Error("welcome email failed", "user", user.ID, "err", err) // ← seen, not lost
		}
	}()

For anything that must happen (emails, webhooks, billing), the more robust fix is a job queue: write "send welcome email to user 42" to a database table or queue, and let a separate worker retry it until it succeeds. Then a crash or deploy can't lose it.

3 · Unlimited concurrency

for _, url := range urls { // 100,000 URLs
	go fetch(url)           // ✗ 100,000 connections at once
}

Each goroutine is cheap, but what it uses isn't: sockets, memory, the other server's patience. Starting everything at once is how you get "too many open files" errors and get rate-limited or blocked. The cure, a fixed number of workers pulling from a queue, is the subject of Worker pools.

The same mistakes, system-sized

This section only names the system-level anti-patterns so you can recognize them in design discussions. Later architecture topics go deeper.

The distributed monolith, up close

This one deserves a picture, because teams usually walk into it while trying to fix a big ball of mud. They split the monolith (one big program, deployed as a single unit) into separate services along the wrong lines, keep one shared database, and wire the services together with chains of synchronous calls (the caller waits for each answer before it can continue).

The result has the downsides of both worlds. Every function call that used to take nanoseconds is now a network call that can be slow or fail. A change to a table's columns in the shared database breaks three services at once. And because each feature still needs all four services changed, every release is a coordinated deploy of all of them, which is exactly what services were supposed to avoid.

The usual warning signs:

• You can't deploy service A without also deploying B.
• Several services read and write the same tables.
• A single user request makes a long chain of calls, A → B → C → D, and fails if any one of them is down.
• "Which service owns this rule?" gets the answer "kind of all of them".

Practice: review this code

Below are two functions that each contain most of the smells from §02–§08. Click every line you'd comment on in a code review, then press Check. You'll see what you found (green), what you missed (orange) and any false alarms (grey). Click any highlighted line afterwards for the explanation and the fix.

1var db *sql.DB // set once in main()
2 
3func ProcessOrder(
4    orderID, userID string,
5    notify bool,
6    rush bool,
7) error {
8    row := db.QueryRow("SELECT total, country, status FROM orders WHERE id = $1", orderID)
9    var total float64
10    var country, status string
11    if err := row.Scan(&total, &country, &status); err != nil {
12        if strings.Contains(err.Error(), "no rows") {
13            return errors.New("order not found")
14        }
15        return err
16    }
17    if status == "pending" {
18        total = total * 1.1
19    }
20    if country != "VN" {
21        total += 15
22    }
23    if notify {
24        go sendEmail(userID, total)
25    }
26    time.Sleep(200 * time.Millisecond) // give the email time to send
27    return nil
28}

How to use this in a real review

You won't comment on ten smells in a colleague's pull request, and you shouldn't. A review with ten nitpicks buries the one comment that matters. A useful order of priority:

1. Bugs waiting to happen: fire-and-forget work, sleep-as-synchronization, float money, string-matched errors. These fail in production.
2. Things that will be hard to change later: new global state, a new flag argument on a widely used function, a leaky interface. These get more expensive every week they stay.
3. Readability: magic numbers, a name that could be better. Mention them briefly, or fix them yourself in a follow-up.

Fixing anti-patterns safely

Knowing a smell isn't the hard part. The hard part is fixing it in a codebase that's running in production, without breaking things and without stopping feature work for a month. Four habits make that possible.

1 · Pick your battles: find the hotspots

Messy code only costs you when someone has to read or change it. A messy file nobody touches costs almost nothing. So rank files by two numbers together: how big (or complex) they are, and how often they change. That second number is called churnChurnHow often a file changes, usually counted as the number of commits that touched it in some period (e.g. the last 12 months).. A file that scores high on both is a hotspotHotspotA file that is both large (or complicated) and changes often. It's where messy code actually costs you time, so it's the best place to start cleaning up..

# The 10 most-changed files in the last year
git log --since="12 months ago" --name-only --format="" | sort | uniq -c | sort -rn | head -10

In most codebases a handful of files take most of the changes, so cleaning up those few pays back far more than tidying everything a little.

2 · Pin the behavior: characterization tests

Before changing structure, you need to know if you changed behavior. Legacy code rarely has good tests, and you may not even know what the "correct" behavior is. So don't test what it should do. Record what it does, with a characterization testCharacterization testA test that records what the code does today — right or wrong — so you can refactor and immediately see if anything changed. Also called a golden-master or snapshot test.. Here is one for an old InvoiceTotal function. (It's legacy code, not §06's Order.Total(): it has its own Order shape and returns formatted text like "19.99 USD".)

func TestInvoiceTotal_Characterization(t *testing.T) {
	cases := []struct {
		name  string
		order Order
		want  string // ← copied from what the code prints TODAY
	}{
		{"one item", Order{Items: []Item{{PriceCents: 1999, Qty: 1}}}, "19.99 USD"},
		{"vip discount", Order{VIP: true, Items: []Item{{PriceCents: 1000, Qty: 3}}}, "27.00 USD"},
		{"empty order", Order{}, "0.00 USD"},
		{"odd rounding", Order{VIP: true, Items: []Item{{PriceCents: 5, Qty: 1}}}, "0.04 USD"}, // ← looks wrong! keep it for now
	}
	for _, c := range cases {
		t.Run(c.name, func(t *testing.T) {
			if got := InvoiceTotal(c.order); got != c.want {
				t.Errorf("InvoiceTotal() = %q, want %q", got, c.want)
			}
		})
	}
}

Notice the "odd rounding" case. If today's code does something surprising, record it anyway. Your refactoring must not change it. Fixing that bug is a separate, deliberate change that someone signs off on, because a customer somewhere may depend on the old behavior.

3 · Move in small steps

RefactoringRefactoringChanging how code is structured without changing what it does — renaming, splitting functions, moving code — usually in small, tested steps. means changing structure without changing behavior. Do it in steps so small that each one is obviously safe:

1. One kind of move per commit: rename, or move a method, or extract a type, never all three at once.
2. Run the tests after every step. If they fail, undo the step (don't debug it), and take a smaller one.
3. Never mix refactoring and behavior changes in one pull request. Reviewers can check "nothing changed" quickly, but "nothing changed except this one fix" is much harder to check.
4. Let the tools do it. Editors (VS Code, GoLand) and gopls rename (Go's official editor helper, run from the command line) can rename and move code across a whole project without typos.

4 · Replace big things gradually: the strangler fig

When a whole module needs replacing, the tempting move is a rewrite: build the new version on the side and switch over on one big day. Rewrites are famous for running late, because the old code holds years of edge cases nobody wrote down.

The safer alternative is the strangler figStrangler figA way to replace old code gradually: put a thin layer in front of it, move one feature at a time to new code behind that layer, and delete the old code once nothing reaches it. Named after a vine that slowly grows around a tree., named after a vine that grows around a tree until it can stand on its own. You put a thin layer in front of the old code, then move one piece at a time behind it:

Two ideas make this work, and the walkthrough below uses both. A seamSeamA place where you can swap one piece of a program for another without editing the code around it. An injected dependency is a seam: tests plug in a fake there. (below, the router) is one place where you can redirect calls without editing the code on either side. A feature flagFeature flagA runtime on/off switch (a config value) that decides whether new code runs, so you can ship code turned off and enable it gradually — or turn it off instantly if it breaks. lets you turn the new path on for 1% of users, then 10%, then everyone, and switch it off in seconds if something breaks.

1mux := http.NewServeMux()
2 
3mux.Handle("/invoices/", newInvoices) // moved in step 3
4mux.Handle("/refunds/", newRefunds)   // moved in step 5
5 
6mux.Handle("/", legacyApp)            // everything else: old code
7 
8http.ListenAndServe(":8080", mux)

When NOT to fix it

Sometimes the right call is to leave an anti-pattern exactly where it is:

• It's cold. Big, ugly, and barely touched in a year, with no bugs: see report_gen.go in the chart above. Cleaning it costs weeks and saves nothing.
• It's about to be deleted. Don't polish a feature that's being retired next quarter.
• You can't test it yet. Refactoring without a safety net is just editing and hoping. Write characterization tests first, or don't start.
• You don't understand why it's that way. Odd code sometimes exists for a reason: a customer contract, a hardware quirk, a past outage. Ask, or run git blame (it shows which commit last changed each line, and that commit's message often says why), before you "fix" it.

When anti-pattern hunting becomes the anti-pattern

Once you learn the names from this page, you'll start seeing smells everywhere. That's good. Acting on all of them is not. Here are the ways enthusiasm goes wrong.

Check yourself

Answer before you click. Each question is covered above: Q1 in §03, Q2 in §05, Q3 in §04, Q4 in §08, Q5 in §07, Q6 in §11–§12.

Ticket: “Add a middle name to users.”
Files changed: user.go, users.sql, SignupForm.jsx, ProfilePage.jsx,
admin_table.jsx, welcome_email.go, invoice_pdf.go, csv_export.go,
search_index.go, validate.go, api_docs.yaml   (11 files)

type UserID string
type OrderID string

func Cancel(u UserID, o OrderID) {}

func main() {
    var u UserID = "u-1"
    var o OrderID = "o-9"
    Cancel(u, o)          // A
    Cancel("o-9", "u-1")  // B
    Cancel(o, u)          // C
}

var cfg *Config

func main() {
    go func() { cfg = loadConfig() }() // reads a file, ~20 ms on a laptop
    time.Sleep(100 * time.Millisecond)
    fmt.Println(cfg.Port)
}

try {
  await api.getUser(id);
} catch (err) {
  if (err.message === "User Not Found") return null;
  throw err;
}
// The API library v3 changes the message to "user not found".

legacy/report_gen.go
  3,100 lines · 1 god struct · no tests
  commits in the last 2 years: 2 (both typo fixes)
  open bugs: 0

Cheat sheet