Effective Go

Effective Go

A code-review and authoring skill distilling common Go review comments and idiomatic patterns, sourced from pthethanh/effective-go (which itself supplements the official Effective Go and Go Code Review Comments).

When to use

• Reviewing Go code → Walk the checklist below. Flag every violation with the specific rule and a concrete fix.
• Writing new Go code → Apply the patterns proactively. Prefer nil slices, accept ctx as first param, return concrete types, etc.
• Refactoring Go code → Identify which rules the current code violates, then show a before/after.

For any rule where the user needs more depth (rationale, full examples, edge cases), read references/best-practices.md.

Review checklist

Work through these categories when reviewing Go code. Each bullet is a high-signal thing to check.

Formatting & tooling

• Code is gofmt'd (or goimports'd). If not, flag it — this should be automatic in the IDE.
• go vet ./... passes. Suggest wiring into a Makefile along with fmt, test, build.

Naming

• Package names: short, lowercase, no util / common / misc / api / types / interfaces. Clients write chubby.File, not chubby.ChubbyFile — don't stutter.
• MixedCaps, never snake_case or SCREAMING_CASE. Unexported constant is maxLength, not MAX_LENGTH.
• Initialisms keep consistent case: ServeHTTP not ServeHttp, userID not userId, XMLHTTPRequest.
• Variable names: short for narrow scope (i, r, c), longer the further from declaration. Receiver names are 1–2 letters and consistent across all methods (c or cl for Client, never me / this / self).

Comments & docs

• All exported names have doc comments. Comments are full sentences that start with the name and end with a period: // Encode writes the JSON encoding of req to w.
• Error strings are lowercase, no trailing punctuation: fmt.Errorf("something bad"), not "Something bad.". (Logging is exempt.)
• Package comment is adjacent to the package clause with no blank line, starts with Package foo ... (or for main: Binary foo ... / Command foo ...).

Errors

• Never discard errors with _ unless there's a documented reason. Check, return, or (rarely) panic.
• Don't panic for normal error handling.
• Check error type, not string contents. Use errors.As / errors.Is (Go 1.13+) or type assertions, never strings.Contains(err.Error(), "timeout").
• Wrap errors with context: fmt.Errorf("open %s: %w", path, err) (or errors.Wrap pre-1.13). Preserve the root error for type checks.
• Indent error flow, not success. if err != nil { return err } first, then continue at base indentation. No else branch for the happy path.
• No in-band errors (-1, "", nil as sentinels). Use multiple returns: (value, ok) or (value, error).

Interfaces

• Interfaces belong on the consumer side, not the producer. The package that uses the interface defines it; the package that implements it returns a concrete type.
• Don't define interfaces before they're used. No speculative interfaces "for mocking" — design the API so real implementations can be tested directly.
• Return concrete types from constructors (e.g. func NewThinger() *Thinger, not func NewThinger() Thinger). Consumers can wrap in their own interface as needed.

Concurrency & context

• context.Context is the first parameter of any function that needs it, named ctx: func F(ctx context.Context, ...).
• Never store ctx on a struct. Pass it through each call. (Rare exception: matching a stdlib interface.)
• Don't define custom Context types or context-like interfaces.
• Goroutine lifetimes must be obvious — it's clear when/whether they exit. Document it when not. Leaked goroutines = silent memory bloat + panics on closed channels.
• Prefer synchronous functions. Let the caller add concurrency if they want it. Easier to test, reason about, and leak-proof.

Functions & methods

• Pass values, not pointers, unless (a) the struct is large, (b) the method mutates the receiver, (c) the struct contains a sync.Mutex, or (d) you need nil. Don't *string / *io.Reader "to save bytes."
• Receiver type rules of thumb: map/func/chan → value. Mutates receiver → pointer. Contains a mutex → pointer. Large struct → pointer. Small, immutable, basic-type → value. When in doubt → pointer, and be consistent within a type (don't mix value and pointer receivers).
• Named result parameters only when they add clarity to godoc (e.g. (lat, long float64, err error)) or when needed for deferred closures. Don't use them just to enable naked returns. Naked returns are only OK in short functions.

Data & memory

• Empty slices: prefer var t []string (nil) over t := []string{} (non-nil zero-length). Exception: encoding JSON where you need [] instead of null.
• Don't copy structs whose methods hang off the pointer type (*T). Classic gotcha: bytes.Buffer.
• Use crypto/rand for keys, tokens, anything security-adjacent. math/rand is predictable even when seeded with time.Now().

HTTP & production

• Never use http.DefaultClient or http.Get in production. Default timeout is 0 = infinite hang. Define a custom &http.Client{Timeout: ...} and reuse it.
• Never use the zero-value http.Server either. Set ReadTimeout, WriteTimeout, IdleTimeout.

Architecture

• Avoid global variables. Inject dependencies into structs via constructor; wire them up in main. A global *sql.DB makes tests painful and blast radius unclear.
• Project layout: for apps, follow golang-standards/project-layout as a starting point. If you can't easily write unit tests for your code, the design is too coupled — fix the design, not the tests.

Testing

• Useful failure messages: include the input, what was got, what was expected. Order is got != want, and the Errorf format mirrors that: t.Errorf("Foo(%q) = %d; want %d", in, got, want). Not "expected X got Y."
• Table-driven tests for multiple input cases. Wrap each caller in a differently-named TestFoo function if you need per-case test names in output.
• Write runnable Example functions for new packages — they double as docs and executable tests.

Imports

• Grouped with blank lines between groups: stdlib first, then third-party, then local. goimports handles this.
• Don't rename imports except to resolve a collision. Rename the most local import when you do.

Output format

When reviewing code, structure feedback as:

1. Summary — 1–2 lines on overall state.
2. Findings, grouped by severity (blocker / should-fix / nit). Each finding cites the rule name (e.g. "Indent error flow", "Don't use default http.Client"), quotes the offending line, and shows the fix.
3. Refactored version if the user asked for one, or offered as a follow-up.

When writing new Go, just write it correctly and briefly note any non-obvious choice (e.g. "returning concrete *Client, not an interface — consumers can wrap for mocking").

Reference

references/best-practices.md contains the full rationale and extended examples for every rule above. Read it when a rule needs more than the checklist gives you — especially for receiver type decisions, context conventions, error wrapping, and HTTP timeout configuration.