Idioms, Style and Code Review

The shelf catalogue shows what "the package name is part of every use" means in practice:

Two of these rules produce compile-time or run-time differences you can see. The error-string rule is purely about how messages compose:

The first message composes into loading shelf: open catalogue: file missing, which reads as one sentence; the second shows a capital letter and a full stop in the middle of a line. The standard library follows the same rule, as the last line shows (open /nonexistent/shelf.json: no such file or directory).

Source: Go Code Review Comments: Error Strings

An in-band error is a value from the normal result range that means "failed". The risk is that it flows on into code that treats it as data. The fix is a second result, and the compiler then forces the caller to deal with it:

Source: Go Code Review Comments: In-Band Errors

LookupBad("weight") returns "" for a missing key, and strconv.Atoi then reports a parse failure, pointing the reader at the wrong problem. Lookup returns (string, bool), and Pages turns the missing key into an error that says what was missing. Trying to pass both results of a two-value function to a one-value function is a compile error:

Passing the results of a two-result call straight to another function is allowed only when the parameter list matches exactly, which strconv.Atoi(string) does not. Not every special value is in-band error signalling: strings.Index returns -1 for "not found" and strings.Cut returns (before, after string, found bool). The standard library has both, and the second form is the recommended one for new code.

Source: Go Code Review Comments: In-Band Errors

Indent error flow is the most visible habit in Go source. Both functions below behave identically; the second is the idiom:

pagesNested has an else after every return, so the successful path is buried two levels deep. pages is flat: each failure is dealt with and left behind, and the last line is the result. The program also shows two further idioms: the enum starts with an explicit Unknown zero value, so an unset LoanState is distinguishable from a real one, and var _ fmt.Stringer = LoanState(0) makes the compiler check that the type implements the interface (a compile-time assertion that costs nothing at run time).

Source: Go Code Review Comments: Indent Error Flow and Effective Go: If

Receiver kind is the item that produces a go vet report. A value receiver on a type that contains a mutex copies the lock on every call, and so does an assignment:

Both reports come from the copylocks analyzer. The fix is a pointer receiver on Value (and never copying a Counter). The program still compiles and prints 1 1, because the copy happens to be taken when the mutex is unlocked; that is exactly why this is a review item and not only a bug.

Source: Go Code Review Comments: Receiver Type and cmd/vet

A nil slice is the correct empty value, with one visible exception:

len, cap, range and append treat both forms alike. Only comparison with nil and JSON encoding tell them apart, which is why an API should not give the two forms different meanings, and why a handler that must send [] creates the slice with make or a literal.

Source: Go Code Review Comments: Declaring Empty Slices

Keys, tokens and identifiers that must be unguessable come from crypto/rand. Since Go 1.24 crypto/rand.Text returns a ready-to-use random string, so there is no need for an encoding step:

Text returns 26 characters of the RFC 4648 base32 alphabet, which is 128 bits of randomness; the program prints only the length and a comparison because the value is different on every run. math/rand/v2 is the right package for simulations, shuffles and tests, and the wrong one for anything an attacker would like to predict. See Crypto and Security.

Source: crypto/rand.Text and Go 1.24 release notes

The import grouping and the dot-import rule are mechanical. The test rule is easiest to see in a failing run, which is shown after the next section.

A type is easiest to use when var x T already works. The standard library does this everywhere: sync.Mutex, sync.WaitGroup, bytes.Buffer, strings.Builder and http.Server can be used without a constructor. The benefits are fewer invalid states, no New function to forget, and structs that embed such a type become usable too.

Shelf allocates its map lazily inside Add; reading a nil map is fine, writing needs the allocation. The last two lines show the limit of the idiom: a bare map variable is not ready to write, and writing to it panics with the message assignment to entry in nil map. So the zero value is useful when the type’s methods take care of that, and when they cannot, a constructor (NewMemStore in the shelf module) is the honest answer. A type that must hold a mutex should be documented as not copyable (see Interfaces, types and values).

Source: Effective Go: Allocation with new and Go Proverbs (community)

Related practice from the guides: start an iota enum so that the zero value means "unset" (shown in the earlier LoanState example) unless zero is a valid and meaningful member. Uber’s guide recommends starting at one for the same reason; the shelf module’s own catalog.LoanState starts at Available because "available" is the sensible default.

The rule is a shorthand for three review comments: a function should depend on the smallest behaviour it needs (a parameter of interface type), it should hand back a concrete type so new methods can be added without breaking callers, and the interface should be declared by the consumer. The Code Review Comments page says it directly: define the interface in the package that uses it.

MemStore is the producer. It declares no interface, so adding methods (Len, Put) breaks nobody. Titles is the consumer: it declares bookGetter with the single method it calls, which means any value with a Get method works, including fakeGetter, a test double written in two lines without any help from the producer. This is also why Go interfaces are so small: the standard library’s most useful ones (io.Reader, io.Writer, fmt.Stringer, error, sort.Interface) have one to three methods.

The exceptions are real and common:

The opposite habit, declaring an interface next to every implementation "so it can be mocked", is the second thing the reviewers push back on, because it freezes the method set and guesses at needs nobody has yet. When you do need a fake, declare the small interface in the test file. See Interfaces and Composition and Object-Oriented Design.

Source: Go Code Review Comments: Interfaces and Effective Go: Interfaces and other types

Bodner’s advice for beginners coming from Java or C# is to start with values and add pointers only when there is a reason. The reasons that qualify:

The reasons that do not: "to avoid copying a string" (a string is two words), "because that is what the other language does" and "so the function can return nil on error" (return an error). Pointers also have costs: they can be nil, they make escape analysis keep values on the heap (see Memory Management and Garbage Collection), and they make it harder to see who can modify a value.

The most common misuse is the pointer to an interface. An interface value already is a small header that can hold a pointer, so *io.Reader is almost never what you want, and the compiler says so:

The fix is to take io.Reader and call drain(r) with the interface value itself. Pointers to maps, slices and channels are equally unusual: a map, a channel and a function value are already references, and a slice header is passed by value deliberately (when a method must grow a slice, return the new slice, as append does).

Go 1.26 also made one old pointer chore unnecessary: new now accepts an expression, so p := new(30) or new(time.Now()) creates a pointer to an initialised value, replacing the one-line helper that older code defined to take the address of a literal.

Source: Go Code Review Comments: Pass Values, Language specification: Allocation and Go 1.26 release notes

See Pointers and Value Semantics for the full rules, and for comparison the C Reference on pointers, where the same decision is made without a garbage collector.

A naked return returns whatever the named results currently hold. Bodner’s rule is to avoid it entirely, the Google guide allows it only in small functions, and Uber and the Code Review Comments page do not recommend it. The danger is a shadowed result:

The inner n, err := strconv.Atoi(s) declares new variables, so the return inside the if would return the outer results unchanged, and the compiler rejects it. In real code the shadowing is often subtler (a variable declared in an if header) and nothing complains. The rule of thumb:

Named results used together with defer are an idiom, not a style problem; see Functions, Closures and Defer and Error Handling.

Source: Go Code Review Comments: Named Result Parameters and Language specification: Return statements

A function that starts a goroutine and returns a channel decides for the caller how the work is scheduled, when it stops and who cleans up. A synchronous function leaves all of those decisions to the caller, who can add concurrency in one line and cannot take it away.

Source: Go Code Review Comments: Synchronous Functions and sync: WaitGroup.Go

FetchTitle is synchronous and tells the caller everything: the result, the error, and that it is finished when it returns. The three calls run concurrently because the caller chose to wrap them in wg.Go (Go 1.25). FetchTitleAsync hides a goroutine behind an unbuffered channel; if the caller never receives, that goroutine blocks forever, and nothing in the signature offers cancellation or an error. Forms of the same trap: returning a channel that someone must drain, starting a background goroutine in a constructor with no Close, and callbacks invoked on an unspecified goroutine.

When concurrency is the feature (a server, a pipeline), the API says so, accepts a context.Context and gives the caller a way to wait: a Run(ctx) that blocks until the context is cancelled, or a Close that stops the workers and returns. A goroutine whose lifetime is bound to a context can be tested for a clean exit:

watchLeaky starts five goroutines on channels nobody closes; five goroutines remain, which runtime.NumGoroutine shows (the number is for this run; the point is that it equals the number of leaked calls). watch returns a done channel that is closed when the goroutine exits, and it exits when the context is cancelled or the input channel closes. This is the shape recommended by the Code Review Comments page: make clear when, or whether, goroutines exit. See Concurrency Patterns, Context and Goroutines and the Scheduler.

Source: Go Code Review Comments: Synchronous Functions, Goroutine Lifetimes and sync.WaitGroup.Go

init functions run automatically when a package is initialised, in dependency order, before main. They cannot take arguments, cannot return an error, cannot be called again, and run for every importer, including programs that do not use the feature. The guides (Uber says "avoid `init()`", Bodner says to avoid it except for registering drivers) give reasons that are easy to see:

The init version reads the environment on import and silently falls back to a default path; a test cannot exercise a different environment without setting variables before the process starts, and the failure case (a missing variable) cannot be reported. ConfigFromEnv takes the lookup function as a parameter, returns an error and is trivially testable. The same applies to package-level variables initialised with function calls that can fail or depend on global state.

Legitimate uses are narrow: registering a driver or codec with a registry in a package that exists only for that purpose (database/sql drivers, image/png), precomputing a table, and a handful of other things that cannot fail. The corresponding Code Review Comments rule is that a blank import belongs only in main or in tests. Initialisation order across package-level variables and several init functions is specified; see Packages, Imports and Initialization.

Source: Language specification: Package initialization and Uber Go Style Guide (community)

net/http offers convenient package-level values: http.Get and http.DefaultClient, http.Handle, http.HandleFunc and http.DefaultServeMux, and http.ListenAndServe(addr, nil). Bodner’s advice is never to use them in production code, and the reasons have not changed:

The first line shows that the default client has a zero timeout. The purpose-built client times out in 100 ms (the Timeout() bool method is the portable way to ask whether an error is a timeout) and is a distinct value you can configure once and share. The private ServeMux is an ordinary value passed to the server, and since Go 1.22 it routes by method and wildcard (GET /books/{isbn}) so the router that older books recommend a third-party library for is built in. The last lines show the hazard: a handler registered with http.HandleFunc is visible on DefaultServeMux, which a nil-handler server serves. For production servers set ReadHeaderTimeout, ReadTimeout, WriteTimeout and IdleTimeout on an explicit http.Server; the shelf service does this, and net/http Clients and Servers covers the details, including CrossOriginProtection (Go 1.25) and, for CORS, the site’s CORS page.

Source: net/http.Client, net/http.ServeMux and Routing Enhancements for Go 1.22

An interface of one to three methods can be satisfied by many types, composed (io.ReadWriter), and faked in a test. A large interface forces every implementation and fake to implement what the caller never calls. Two rules follow. First, the consumer defines the interface it needs (see Accept interfaces, return structs). Second, when the producer must offer an interface (a plug-in point), keep it minimal, and add optional capabilities through additional interfaces discovered by type assertion, as io.WriterTo and http.Flusher do, rather than by growing the original.

A constructor with many optional settings should not need a dozen parameters or force callers to fill a struct with zero values that mean "default". The two accepted answers are a plain config struct (best when there are few, stable settings and the zero value of each field is a sensible default) and functional options (best when settings are many, may grow, or need validation):

Each Option is a function that modifies the client being built and may return an error, which NewClient wraps with context. Defaults are visible in one place, new options can be added without breaking callers, and call sites read as NewClient(url, WithTimeout(2*time.Second)). The cost is more code and one function per setting; for a handful of fields a documented Config struct (declared at the bottom of the example) is simpler. Bodner presents the struct as the way to simulate named parameters, and the options pattern for libraries that must stay extensible. Do not make options a reflex: an unexported *Client field with no option is not a bug.

Source: Uber Go Style Guide: Functional Options (community) and Effective Go: Composite literals

Which errors a function can return is as much a part of its contract as its parameters. Callers can only depend on what the documentation promises:

errors.AsType[*ValidationError](err) returns the typed error and a bool without the target variable that errors.As needs. find documents ErrNotFound and *ValidationError as part of its contract; the wrapped message find "404": catalog: not found adds context but keeps errors.Is working. The Go blog post Working with Errors in Go 1.13 is the design rationale.

Source: errors.AsType and Working with Errors in Go 1.13

A function that can block, call another service or be cancelled takes ctx context.Context as its first parameter and passes it down. It never stores the context in a struct (the lifetime of a request is not the lifetime of the object), and it never takes a custom context type.

Find selects on ctx.Done() and reports context.Cause(ctx), which is context deadline exceeded here and would be the custom error if the caller used WithCancelCause. The exception noted by the Go team is a method whose signature is fixed by an interface in the standard library (http.Handler gets its context from r.Context()). A struct that keeps a context is documented in the blog post Contexts and structs as a smell, with the few exceptions. go vet's lostcancel analyzer reports a cancel function that is never called. Details are in Context.

Source: Go Code Review Comments: Contexts and Contexts and structs

Every exported name needs a doc comment, and go doc shows exactly what the next developer will read. For the documented type in the demonstration package:

The comments are sentences that start with the name (Loan records a book..., Return marks the loan...) and the field comment documents the invariant callers need. See Documentation and Doc Comments.

Source: Go Doc Comments

The Google Go Style Guide (https://google.github.io/styleguide/go/) is three documents: the guide (normative: the principles from the earlier section and the core rules), decisions (normative for readability reviewers at Google, with reasoning) and best practices (not normative). Its parts worth knowing:

Its main difference in tone from the wiki page is explicit reasoning for each rule, and its explicit ranking of the principles. It treats consistency inside a package or file as more important than any of the individual rules.

The Uber Go Style Guide (https://github.com/uber-go/guide) is shorter and written as a list of "bad" and "good" snippets. Its recurring points: verify interface compliance at compile time (var _ Store = (*MemStore)(nil), as in the shelf module), zero-value mutexes are valid (do not allocate one), copy slices and maps at API boundaries, defer to clean up, channels have size one or none, start enums at one, use time.Time and time.Duration instead of bare integers, error types and wrapping, handle each error once, handle failed type assertions with the two-value form, do not panic, avoid mutable globals, avoid embedding types in public structs, avoid built-in names as identifiers, avoid init(), exit in main, do not fire and forget goroutines, prefer strconv over fmt for number conversion, specify container capacity, prefer raw string literals, use field names to initialise structs, and use test tables and functional options.

Some Uber items have aged. The recommendation to use the go.uber.org/atomic package assumed that the standard library’s sync/atomic had no typed values; since Go 1.19 atomic.Int64, atomic.Bool, atomic.Pointer[T] and the other typed atomics exist, and the go fix modernizer atomictypes rewrites old function-style calls. The guide’s advice on "start enums at one" and "channel size is one or none" is a team convention, not a language rule.

The proverbs are a mnemonic list, and each one is explained somewhere on this site rather than needing its own rule:

They are compressed advice from 2015 and carry no version information: the proverb about the empty interface predates any, and the one about dependencies predates the module proxy and checksum database.