| Bad | Good |
|---|---|
| + +```go +BAD CODE GOES HERE +``` + + | + +```go +GOOD CODE GOES HERE +``` + + |
| Bad | Good |
|---|---|
| + +```go +type foo struct { + running int32 // atomic +} + +func (f* foo) start() { + if atomic.SwapInt32(&f.running, 1) == 1 { + // already running… + return + } + // start the Foo +} + +func (f *foo) isRunning() bool { + return f.running == 1 // race! +} +``` + + | + +```go +type foo struct { + running atomic.Bool +} + +func (f *foo) start() { + if f.running.Swap(true) { + // already running… + return + } + // start the Foo +} + +func (f *foo) isRunning() bool { + return f.running.Load() +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +var error string +// `error` shadows the builtin + +// or + +func handleErrorMessage(error string) { + // `error` shadows the builtin +} +``` + + | + +```go +var errorMessage string +// `error` refers to the builtin + +// or + +func handleErrorMessage(msg string) { + // `error` refers to the builtin +} +``` + + |
| + +```go +type Foo struct { + // While these fields technically don't + // constitute shadowing, grepping for + // `error` or `string` strings is now + // ambiguous. + error error + string string +} + +func (f Foo) Error() error { + // `error` and `f.error` are + // visually similar + return f.error +} + +func (f Foo) String() string { + // `string` and `f.string` are + // visually similar + return f.string +} +``` + + | + +```go +type Foo struct { + // `error` and `string` strings are + // now unambiguous. + err error + str string +} + +func (f Foo) Error() error { + return f.err +} + +func (f Foo) String() string { + return f.str +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +// Ought to be enough for anybody! +c := make(chan int, 64) +``` + + | + +```go +// Size of one +c := make(chan int, 1) // or +// Unbuffered channel, size of zero +c := make(chan int) +``` + + |
| Bad | Good |
|---|---|
| + +```go +files, _ := os.ReadDir("./files") + +m := make(map[string]os.DirEntry) +for _, f := range files { + m[f.Name()] = f +} +``` + + | + +```go + +files, _ := os.ReadDir("./files") + +m := make(map[string]os.DirEntry, len(files)) +for _, f := range files { + m[f.Name()] = f +} +``` + + |
| + +`m` is created without a size hint; the map will resize +dynamically, causing multiple allocations as it grows. + + | + +`m` is created with a size hint; there may be fewer +allocations at assignment time. + + |
| Bad | Good |
|---|---|
| + +```go +for n := 0; n < b.N; n++ { + data := make([]int, 0) + for k := 0; k < size; k++{ + data = append(data, k) + } +} +``` + + | + +```go +for n := 0; n < b.N; n++ { + data := make([]int, 0, size) + for k := 0; k < size; k++{ + data = append(data, k) + } +} +``` + + |
| + +```plain +BenchmarkBad-4 100000000 2.48s +``` + + | + +```plain +BenchmarkGood-4 100000000 0.21s +``` + + |
| Bad | Good |
|---|---|
| + +```go +func (d *Driver) SetTrips(trips []Trip) { + d.trips = trips +} + +trips := ... +d1.SetTrips(trips) + +// Did you mean to modify d1.trips? +trips[0] = ... +``` + + | ++ +```go +func (d *Driver) SetTrips(trips []Trip) { + d.trips = make([]Trip, len(trips)) + copy(d.trips, trips) +} + +trips := ... +d1.SetTrips(trips) + +// We can now modify trips[0] without affecting d1.trips. +trips[0] = ... +``` + + | +
| Bad | Good |
|---|---|
| + +```go +type Stats struct { + mu sync.Mutex + counters map[string]int +} + +// Snapshot returns the current stats. +func (s *Stats) Snapshot() map[string]int { + s.mu.Lock() + defer s.mu.Unlock() + + return s.counters +} + +// snapshot is no longer protected by the mutex, so any +// access to the snapshot is subject to data races. +snapshot := stats.Snapshot() +``` + + | + +```go +type Stats struct { + mu sync.Mutex + counters map[string]int +} + +func (s *Stats) Snapshot() map[string]int { + s.mu.Lock() + defer s.mu.Unlock() + + result := make(map[string]int, len(s.counters)) + for k, v := range s.counters { + result[k] = v + } + return result +} + +// Snapshot is now a copy. +snapshot := stats.Snapshot() +``` + + |
| Bad | Good |
|---|---|
| + +```go +import "a" +import "b" +``` + + | + +```go +import ( + "a" + "b" +) +``` + + |
| Bad | Good |
|---|---|
| + +```go + +const a = 1 +const b = 2 + + + +var a = 1 +var b = 2 + + + +type Area float64 +type Volume float64 +``` + + | + +```go +const ( + a = 1 + b = 2 +) + +var ( + a = 1 + b = 2 +) + +type ( + Area float64 + Volume float64 +) +``` + + |
| Bad | Good |
|---|---|
| + +```go +type Operation int + +const ( + Add Operation = iota + 1 + Subtract + Multiply + EnvVar = "MY_ENV" +) +``` + + | + +```go +type Operation int + +const ( + Add Operation = iota + 1 + Subtract + Multiply +) + +const EnvVar = "MY_ENV" +``` + + |
| Bad | Good |
|---|---|
| + +```go +func f() string { + red := color.New(0xff0000) + green := color.New(0x00ff00) + blue := color.New(0x0000ff) + + // ... +} +``` + + | + +```go +func f() string { + var ( + red = color.New(0xff0000) + green = color.New(0x00ff00) + blue = color.New(0x0000ff) + ) + + // ... +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +func (c *client) request() { + caller := c.name + format := "json" + timeout := 5*time.Second + var err error + + // ... +} +``` + + | + +```go +func (c *client) request() { + var ( + caller = c.name + format = "json" + timeout = 5*time.Second + err error + ) + + // ... +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +p.Lock() +if p.count < 10 { + p.Unlock() + return p.count +} + +p.count++ +newCount := p.count +p.Unlock() + +return newCount + +// easy to miss unlocks due to multiple returns +``` + + | + +```go +p.Lock() +defer p.Unlock() + +if p.count < 10 { + return p.count +} + +p.count++ +return p.count + +// more readable +``` + + |
| Bad | Good |
|---|---|
| + +```go +var a int +if b { + a = 100 +} else { + a = 10 +} +``` + + | + +```go +a := 10 +if b { + a = 100 +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +// ConcreteList is a list of entities. +type ConcreteList struct { + *AbstractList +} +``` + + | + +```go +// ConcreteList is a list of entities. +type ConcreteList struct { + list *AbstractList +} + +// Add adds an entity to the list. +func (l *ConcreteList) Add(e Entity) { + l.list.Add(e) +} + +// Remove removes an entity from the list. +func (l *ConcreteList) Remove(e Entity) { + l.list.Remove(e) +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +// AbstractList is a generalized implementation +// for various kinds of lists of entities. +type AbstractList interface { + Add(Entity) + Remove(Entity) +} + +// ConcreteList is a list of entities. +type ConcreteList struct { + AbstractList +} +``` + + | + +```go +// ConcreteList is a list of entities. +type ConcreteList struct { + list AbstractList +} + +// Add adds an entity to the list. +func (l *ConcreteList) Add(e Entity) { + l.list.Add(e) +} + +// Remove removes an entity from the list. +func (l *ConcreteList) Remove(e Entity) { + l.list.Remove(e) +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +type Operation int + +const ( + Add Operation = iota + Subtract + Multiply +) + +// Add=0, Subtract=1, Multiply=2 +``` + + | + +```go +type Operation int + +const ( + Add Operation = iota + 1 + Subtract + Multiply +) + +// Add=1, Subtract=2, Multiply=3 +``` + + |
| Description | Code |
|---|---|
| + +**Bad**: Log the error and return it + +Callers further up the stack will likely take a similar action with the error. +Doing so makes a lot of noise in the application logs for little value. + + | + +```go +u, err := getUser(id) +if err != nil { + // BAD: See description + log.Printf("Could not get user %q: %v", id, err) + return err +} +``` + + |
| + +**Good**: Wrap the error and return it + +Callers further up the stack will handle the error. +Use of `%w` ensures they can match the error with `errors.Is` or `errors.As` +if relevant. + + | + +```go +u, err := getUser(id) +if err != nil { + return fmt.Errorf("get user %q: %w", id, err) +} +``` + + |
| + +**Good**: Log the error and degrade gracefully + +If the operation isn't strictly necessary, +we can provide a degraded but unbroken experience +by recovering from it. + + | + +```go +if err := emitMetrics(); err != nil { + // Failure to write metrics should not + // break the application. + log.Printf("Could not emit metrics: %v", err) +} + +``` + + |
| + +**Good**: Match the error and degrade gracefully + +If the callee defines a specific error in its contract, +and the failure is recoverable, +match on that error case and degrade gracefully. +For all other cases, wrap the error and return it. + +Callers further up the stack will handle other errors. + + | + +```go +tz, err := getUserTimeZone(id) +if err != nil { + if errors.Is(err, ErrUserNotFound) { + // User doesn't exist. Use UTC. + tz = time.UTC + } else { + return fmt.Errorf("get user %q: %w", id, err) + } +} +``` + + |
| No error matching | Error matching |
|---|---|
| + +```go +// package foo + +func Open() error { + return errors.New("could not open") +} + +// package bar + +if err := foo.Open(); err != nil { + // Can't handle the error. + panic("unknown error") +} +``` + + | + +```go +// package foo + +var ErrCouldNotOpen = errors.New("could not open") + +func Open() error { + return ErrCouldNotOpen +} + +// package bar + +if err := foo.Open(); err != nil { + if errors.Is(err, foo.ErrCouldNotOpen) { + // handle the error + } else { + panic("unknown error") + } +} +``` + + |
| No error matching | Error matching |
|---|---|
| + +```go +// package foo + +func Open(file string) error { + return fmt.Errorf("file %q not found", file) +} + +// package bar + +if err := foo.Open("testfile.txt"); err != nil { + // Can't handle the error. + panic("unknown error") +} +``` + + | + +```go +// package foo + +type NotFoundError struct { + File string +} + +func (e *NotFoundError) Error() string { + return fmt.Sprintf("file %q not found", e.File) +} + +func Open(file string) error { + return &NotFoundError{File: file} +} + + +// package bar + +if err := foo.Open("testfile.txt"); err != nil { + var notFound *NotFoundError + if errors.As(err, ¬Found) { + // handle the error + } else { + panic("unknown error") + } +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +s, err := store.New() +if err != nil { + return fmt.Errorf( + "failed to create new store: %w", err) +} +``` + + | + +```go +s, err := store.New() +if err != nil { + return fmt.Errorf( + "new store: %w", err) +} +``` + + |
| + +```plain +failed to x: failed to y: failed to create new store: the error +``` + + | + +```plain +x: y: new store: the error +``` + + |
| Bad | Good |
|---|---|
| + +```go +func main() { + body := readFile(path) + fmt.Println(body) +} + +func readFile(path string) string { + f, err := os.Open(path) + if err != nil { + log.Fatal(err) + } + + b, err := io.ReadAll(f) + if err != nil { + log.Fatal(err) + } + + return string(b) +} +``` + + | + +```go +func main() { + body, err := readFile(path) + if err != nil { + log.Fatal(err) + } + fmt.Println(body) +} + +func readFile(path string) (string, error) { + f, err := os.Open(path) + if err != nil { + return "", err + } + + b, err := io.ReadAll(f) + if err != nil { + return "", err + } + + return string(b), nil +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +package main + +func main() { + args := os.Args[1:] + if len(args) != 1 { + log.Fatal("missing file") + } + name := args[0] + + f, err := os.Open(name) + if err != nil { + log.Fatal(err) + } + defer f.Close() + + // If we call log.Fatal after this line, + // f.Close will not be called. + + b, err := io.ReadAll(f) + if err != nil { + log.Fatal(err) + } + + // ... +} +``` + + | + +```go +package main + +func main() { + if err := run(); err != nil { + log.Fatal(err) + } +} + +func run() error { + args := os.Args[1:] + if len(args) != 1 { + return errors.New("missing file") + } + name := args[0] + + f, err := os.Open(name) + if err != nil { + return err + } + defer f.Close() + + b, err := io.ReadAll(f) + if err != nil { + return err + } + + // ... +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +func (s *something) Cost() { + return calcCost(s.weights) +} + +type something struct{ ... } + +func calcCost(n []int) int {...} + +func (s *something) Stop() {...} + +func newSomething() *something { + return &something{} +} +``` + + | + +```go +type something struct{ ... } + +func newSomething() *something { + return &something{} +} + +func (s *something) Cost() { + return calcCost(s.weights) +} + +func (s *something) Stop() {...} + +func calcCost(n []int) int {...} +``` + + |
| Bad | Good |
|---|---|
| + +```go +// package db + +func Open( + addr string, + cache bool, + logger *zap.Logger +) (*Connection, error) { + // ... +} +``` + + | + +```go +// package db + +type Option interface { + // ... +} + +func WithCache(c bool) Option { + // ... +} + +func WithLogger(log *zap.Logger) Option { + // ... +} + +// Open creates a connection. +func Open( + addr string, + opts ...Option, +) (*Connection, error) { + // ... +} +``` + + |
| + +The cache and logger parameters must always be provided, even if the user +wants to use the default. + +```go +db.Open(addr, db.DefaultCache, zap.NewNop()) +db.Open(addr, db.DefaultCache, log) +db.Open(addr, false /* cache */, zap.NewNop()) +db.Open(addr, false /* cache */, log) +``` + + | + +Options are provided only if needed. + +```go +db.Open(addr) +db.Open(addr, db.WithLogger(log)) +db.Open(addr, db.WithCache(false)) +db.Open( + addr, + db.WithCache(false), + db.WithLogger(log), +) +``` + + |
| Bad | Good |
|---|---|
| + +```go +var _s string = F() + +func F() string { return "A" } +``` + + | + +```go +var _s = F() +// Since F already states that it returns a string, we don't need to specify +// the type again. + +func F() string { return "A" } +``` + + |
| Bad | Good |
|---|---|
| + +```go +// sign.go + +var _timeNow = time.Now + +func sign(msg string) string { + now := _timeNow() + return signWithTime(msg, now) +} +``` + + | + +```go +// sign.go + +type signer struct { + now func() time.Time +} + +func newSigner() *signer { + return &signer{ + now: time.Now, + } +} + +func (s *signer) Sign(msg string) string { + now := s.now() + return signWithTime(msg, now) +} +``` + + |
| + +```go +// sign_test.go + +func TestSign(t *testing.T) { + oldTimeNow := _timeNow + _timeNow = func() time.Time { + return someFixedTime + } + defer func() { _timeNow = oldTimeNow }() + + assert.Equal(t, want, sign(give)) +} +``` + + | + +```go +// sign_test.go + +func TestSigner(t *testing.T) { + s := newSigner() + s.now = func() time.Time { + return someFixedTime + } + + assert.Equal(t, want, s.Sign(give)) +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +// foo.go + +const ( + defaultPort = 8080 + defaultUser = "user" +) + +// bar.go + +func Bar() { + defaultPort := 9090 + ... + fmt.Println("Default port", defaultPort) + + // We will not see a compile error if the first line of + // Bar() is deleted. +} +``` + + | + +```go +// foo.go + +const ( + _defaultPort = 8080 + _defaultUser = "user" +) +``` + + |
| Bad | Good |
|---|---|
| + +```go +go func() { + for { + flush() + time.Sleep(delay) + } +}() +``` + + | + +```go +var ( + stop = make(chan struct{}) // tells the goroutine to stop + done = make(chan struct{}) // tells us that the goroutine exited +) +go func() { + defer close(done) + + ticker := time.NewTicker(delay) + defer ticker.Stop() + for { + select { + case <-ticker.C: + flush() + case <-stop: + return + } + } +}() + +// Elsewhere... +close(stop) // signal the goroutine to stop +<-done // and wait for it to exit +``` + + |
| + +There's no way to stop this goroutine. +This will run until the application exits. + + | + +This goroutine can be stopped with `close(stop)`, +and we can wait for it to exit with `<-done`. + + |
| Bad | Good |
|---|---|
| + +```go +func init() { + go doWork() +} + +func doWork() { + for { + // ... + } +} +``` + + | + +```go +type Worker struct{ /* ... */ } + +func NewWorker(...) *Worker { + w := &Worker{ + stop: make(chan struct{}), + done: make(chan struct{}), + // ... + } + go w.doWork() + return w +} + +func (w *Worker) doWork() { + defer close(w.done) + for { + // ... + case <-w.stop: + return + } +} + +// Shutdown tells the worker to stop +// and waits until it has finished. +func (w *Worker) Shutdown() { + close(w.stop) + <-w.done +} +``` + + |
| + +Spawns a background goroutine unconditionally when the user exports this package. +The user has no control over the goroutine or a means of stopping it. + + | + +Spawns the worker only if the user requests it. +Provides a means of shutting down the worker so that the user can free up +resources used by the worker. + +Note that you should use `WaitGroup`s if the worker manages multiple +goroutines. +See [Wait for goroutines to exit](goroutine-exit.md). + + |
| Bad | Good |
|---|---|
| + +```go +import ( + "fmt" + "os" + runtimetrace "runtime/trace" + + nettrace "golang.net/x/trace" +) +``` + + | + +```go +import ( + "fmt" + "os" + "runtime/trace" + + nettrace "golang.net/x/trace" +) +``` + + |
| Bad | Good |
|---|---|
| + +```go +import ( + "fmt" + "os" + "go.uber.org/atomic" + "golang.org/x/sync/errgroup" +) +``` + + | + +```go +import ( + "fmt" + "os" + + "go.uber.org/atomic" + "golang.org/x/sync/errgroup" +) +``` + + |
| Bad | Good |
|---|---|
| + +```go +type Foo struct { + // ... +} + +var _defaultFoo Foo + +func init() { + _defaultFoo = Foo{ + // ... + } +} +``` + + | + +```go +var _defaultFoo = Foo{ + // ... +} + +// or, better, for testability: + +var _defaultFoo = defaultFoo() + +func defaultFoo() Foo { + return Foo{ + // ... + } +} +``` + + |
| + +```go +type Config struct { + // ... +} + +var _config Config + +func init() { + // Bad: based on current directory + cwd, _ := os.Getwd() + + // Bad: I/O + raw, _ := os.ReadFile( + path.Join(cwd, "config", "config.yaml"), + ) + + yaml.Unmarshal(raw, &_config) +} +``` + + | + +```go +type Config struct { + // ... +} + +func loadConfig() Config { + cwd, err := os.Getwd() + // handle err + + raw, err := os.ReadFile( + path.Join(cwd, "config", "config.yaml"), + ) + // handle err + + var config Config + yaml.Unmarshal(raw, &config) + + return config +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +type Handler struct { + // ... +} + + + +func (h *Handler) ServeHTTP( + w http.ResponseWriter, + r *http.Request, +) { + ... +} +``` + + | + +```go +type Handler struct { + // ... +} + +var _ http.Handler = (*Handler)(nil) + +func (h *Handler) ServeHTTP( + w http.ResponseWriter, + r *http.Request, +) { + // ... +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +var ( + // m1 is safe to read and write; + // m2 will panic on writes. + m1 = map[T1]T2{} + m2 map[T1]T2 +) +``` + + | + +```go +var ( + // m1 is safe to read and write; + // m2 will panic on writes. + m1 = make(map[T1]T2) + m2 map[T1]T2 +) +``` + + |
| + +Declaration and initialization are visually similar. + + | + +Declaration and initialization are visually distinct. + + |
| Bad | Good |
|---|---|
| + +```go +m := make(map[T1]T2, 3) +m[k1] = v1 +m[k2] = v2 +m[k3] = v3 +``` + + | + +```go +m := map[T1]T2{ + k1: v1, + k2: v2, + k3: v3, +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +mu := new(sync.Mutex) +mu.Lock() +``` + + | + +```go +var mu sync.Mutex +mu.Lock() +``` + + |
| Bad | Good |
|---|---|
| + +```go +type SMap struct { + sync.Mutex + + data map[string]string +} + +func NewSMap() *SMap { + return &SMap{ + data: make(map[string]string), + } +} + +func (m *SMap) Get(k string) string { + m.Lock() + defer m.Unlock() + + return m.data[k] +} +``` + + | + +```go +type SMap struct { + mu sync.Mutex + + data map[string]string +} + +func NewSMap() *SMap { + return &SMap{ + data: make(map[string]string), + } +} + +func (m *SMap) Get(k string) string { + m.mu.Lock() + defer m.mu.Unlock() + + return m.data[k] +} +``` + + |
| + +The `Mutex` field, and the `Lock` and `Unlock` methods are unintentionally part +of the exported API of `SMap`. + + | + +The mutex and its methods are implementation details of `SMap` hidden from its +callers. + + |
| Bad | Good |
|---|---|
| + +```go +for _, v := range data { + if v.F1 == 1 { + v = process(v) + if err := v.Call(); err == nil { + v.Send() + } else { + return err + } + } else { + log.Printf("Invalid v: %v", v) + } +} +``` + + | + +```go +for _, v := range data { + if v.F1 != 1 { + log.Printf("Invalid v: %v", v) + continue + } + + v = process(v) + if err := v.Call(); err != nil { + return err + } + v.Send() +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +func run(args []string) { + if len(args) == 0 { + panic("an argument is required") + } + // ... +} + +func main() { + run(os.Args[1:]) +} +``` + + | + +```go +func run(args []string) error { + if len(args) == 0 { + return errors.New("an argument is required") + } + // ... + return nil +} + +func main() { + if err := run(os.Args[1:]); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +// func TestFoo(t *testing.T) + +f, err := os.CreateTemp("", "test") +if err != nil { + panic("failed to set up test") +} +``` + + | + +```go +// func TestFoo(t *testing.T) + +f, err := os.CreateTemp("", "test") +if err != nil { + t.Fatal("failed to set up test") +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +// func printInfo(name string, isLocal, done bool) + +printInfo("foo", true, true) +``` + + | + +```go +// func printInfo(name string, isLocal, done bool) + +printInfo("foo", true /* isLocal */, true /* done */) +``` + + |
| Bad | Good |
|---|---|
| + +```go +msg := "unexpected values %v, %v\n" +fmt.Printf(msg, 1, 2) +``` + + | + +```go +const msg = "unexpected values %v, %v\n" +fmt.Printf(msg, 1, 2) +``` + + |
| Bad | Good |
|---|---|
| + + ```go + if x == "" { + return []int{} + } + ``` + + | + + ```go + if x == "" { + return nil + } + ``` + + |
| Bad | Good |
|---|---|
| + + ```go + func isEmpty(s []string) bool { + return s == nil + } + ``` + + | + + ```go + func isEmpty(s []string) bool { + return len(s) == 0 + } + ``` + + |
| Bad | Good |
|---|---|
| + + ```go + nums := []int{} + // or, nums := make([]int) + + if add1 { + nums = append(nums, 1) + } + + if add2 { + nums = append(nums, 2) + } + ``` + + | + + ```go + var nums []int + + if add1 { + nums = append(nums, 1) + } + + if add2 { + nums = append(nums, 2) + } + ``` + + |
| Bad | Good |
|---|---|
| + +```go +for i := 0; i < b.N; i++ { + s := fmt.Sprint(rand.Int()) +} +``` + + | + +```go +for i := 0; i < b.N; i++ { + s := strconv.Itoa(rand.Int()) +} +``` + + |
| + +```plain +BenchmarkFmtSprint-4 143 ns/op 2 allocs/op +``` + + | + +```plain +BenchmarkStrconv-4 64.2 ns/op 1 allocs/op +``` + + |
| Bad | Good |
|---|---|
| + +```go +for i := 0; i < b.N; i++ { + w.Write([]byte("Hello world")) +} +``` + + | + +```go +data := []byte("Hello world") +for i := 0; i < b.N; i++ { + w.Write(data) +} +``` + + |
| + +```plain +BenchmarkBad-4 50000000 22.2 ns/op +``` + + | + +```plain +BenchmarkGood-4 500000000 3.25 ns/op +``` + + |
| Bad | Good |
|---|---|
| + +```go +wantError := "unknown name:\"test\"" +``` + + | + +```go +wantError := `unknown error:"test"` +``` + + |
| Bad | Good |
|---|---|
| + +```go +type Client struct { + version int + http.Client +} +``` + + | + +```go +type Client struct { + http.Client + + version int +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +type A struct { + // Bad: A.Lock() and A.Unlock() are + // now available, provide no + // functional benefit, and allow + // users to control details about + // the internals of A. + sync.Mutex +} +``` + + | + +```go +type countingWriteCloser struct { + // Good: Write() is provided at this + // outer layer for a specific + // purpose, and delegates work + // to the inner type's Write(). + io.WriteCloser + + count int +} + +func (w *countingWriteCloser) Write(bs []byte) (int, error) { + w.count += len(bs) + return w.WriteCloser.Write(bs) +} +``` + + |
| + +```go +type Book struct { + // Bad: pointer changes zero value usefulness + io.ReadWriter + + // other fields +} + +// later + +var b Book +b.Read(...) // panic: nil pointer +b.String() // panic: nil pointer +b.Write(...) // panic: nil pointer +``` + + | + +```go +type Book struct { + // Good: has useful zero value + bytes.Buffer + + // other fields +} + +// later + +var b Book +b.Read(...) // ok +b.String() // ok +b.Write(...) // ok +``` + + |
| + +```go +type Client struct { + sync.Mutex + sync.WaitGroup + bytes.Buffer + url.URL +} +``` + + | + +```go +type Client struct { + mtx sync.Mutex + wg sync.WaitGroup + buf bytes.Buffer + url url.URL +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +k := User{"John", "Doe", true} +``` + + | + +```go +k := User{ + FirstName: "John", + LastName: "Doe", + Admin: true, +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +user := User{ + FirstName: "John", + LastName: "Doe", + MiddleName: "", + Admin: false, +} +``` + + | + +```go +user := User{ + FirstName: "John", + LastName: "Doe", +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +sval := T{Name: "foo"} + +// inconsistent +sptr := new(T) +sptr.Name = "bar" +``` + + | + +```go +sval := T{Name: "foo"} + +sptr := &T{Name: "bar"} +``` + + |
| Bad | Good |
|---|---|
| + +```go +type Stock struct { + Price int + Name string +} + +bytes, err := json.Marshal(Stock{ + Price: 137, + Name: "UBER", +}) +``` + + | + +```go +type Stock struct { + Price int `json:"price"` + Name string `json:"name"` + // Safe to rename Name to Symbol. +} + +bytes, err := json.Marshal(Stock{ + Price: 137, + Name: "UBER", +}) +``` + + |
| Bad | Good |
|---|---|
| + +```go +user := User{} +``` + + | + +```go +var user User +``` + + |
| Bad | Good |
|---|---|
| + +```go +// func TestSplitHostPort(t *testing.T) + +host, port, err := net.SplitHostPort("192.0.2.0:8000") +require.NoError(t, err) +assert.Equal(t, "192.0.2.0", host) +assert.Equal(t, "8000", port) + +host, port, err = net.SplitHostPort("192.0.2.0:http") +require.NoError(t, err) +assert.Equal(t, "192.0.2.0", host) +assert.Equal(t, "http", port) + +host, port, err = net.SplitHostPort(":8000") +require.NoError(t, err) +assert.Equal(t, "", host) +assert.Equal(t, "8000", port) + +host, port, err = net.SplitHostPort("1:8") +require.NoError(t, err) +assert.Equal(t, "1", host) +assert.Equal(t, "8", port) +``` + + | + +```go +// func TestSplitHostPort(t *testing.T) + +tests := []struct{ + give string + wantHost string + wantPort string +}{ + { + give: "192.0.2.0:8000", + wantHost: "192.0.2.0", + wantPort: "8000", + }, + { + give: "192.0.2.0:http", + wantHost: "192.0.2.0", + wantPort: "http", + }, + { + give: ":8000", + wantHost: "", + wantPort: "8000", + }, + { + give: "1:8", + wantHost: "1", + wantPort: "8", + }, +} + +for _, tt := range tests { + t.Run(tt.give, func(t *testing.T) { + host, port, err := net.SplitHostPort(tt.give) + require.NoError(t, err) + assert.Equal(t, tt.wantHost, host) + assert.Equal(t, tt.wantPort, port) + }) +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +func TestComplicatedTable(t *testing.T) { + tests := []struct { + give string + want string + wantErr error + shouldCallX bool + shouldCallY bool + giveXResponse string + giveXErr error + giveYResponse string + giveYErr error + }{ + // ... + } + + for _, tt := range tests { + t.Run(tt.give, func(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + if tt.shouldCallX { + xMock.EXPECT().Call().Return( + tt.giveXResponse, tt.giveXErr, + ) + } + yMock := ymock.NewMockY(ctrl) + if tt.shouldCallY { + yMock.EXPECT().Call().Return( + tt.giveYResponse, tt.giveYErr, + ) + } + + got, err := DoComplexThing(tt.give, xMock, yMock) + + // verify results + if tt.wantErr != nil { + require.EqualError(t, err, tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, want, got) + }) + } +} +``` + + | + +```go +func TestShouldCallX(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + xMock.EXPECT().Call().Return("XResponse", nil) + + yMock := ymock.NewMockY(ctrl) + + got, err := DoComplexThing("inputX", xMock, yMock) + + require.NoError(t, err) + assert.Equal(t, "want", got) +} + +func TestShouldCallYAndFail(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + + yMock := ymock.NewMockY(ctrl) + yMock.EXPECT().Call().Return("YResponse", nil) + + _, err := DoComplexThing("inputY", xMock, yMock) + assert.EqualError(t, err, "Y failed") +} +``` + |
| Bad | Good |
|---|---|
| + +```go +func isActive(now, start, stop int) bool { + return start <= now && now < stop +} +``` + + | + +```go +func isActive(now, start, stop time.Time) bool { + return (start.Before(now) || start.Equal(now)) && now.Before(stop) +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +func poll(delay int) { + for { + // ... + time.Sleep(time.Duration(delay) * time.Millisecond) + } +} + +poll(10) // was it seconds or milliseconds? +``` + + | + +```go +func poll(delay time.Duration) { + for { + // ... + time.Sleep(delay) + } +} + +poll(10*time.Second) +``` + + |
| Bad | Good |
|---|---|
| + +```go +// {"interval": 2} +type Config struct { + Interval int `json:"interval"` +} +``` + + | + +```go +// {"intervalMillis": 2000} +type Config struct { + IntervalMillis int `json:"intervalMillis"` +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +t := i.(string) +``` + + | + +```go +t, ok := i.(string) +if !ok { + // handle the error gracefully +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +var s = "foo" +``` + + | + +```go +s := "foo" +``` + + |
| Bad | Good |
|---|---|
| + +```go +func f(list []int) { + filtered := []int{} + for _, v := range list { + if v > 10 { + filtered = append(filtered, v) + } + } +} +``` + + | + +```go +func f(list []int) { + var filtered []int + for _, v := range list { + if v > 10 { + filtered = append(filtered, v) + } + } +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +err := os.WriteFile(name, data, 0644) +if err != nil { + return err +} +``` + + | + +```go +if err := os.WriteFile(name, data, 0644); err != nil { + return err +} +``` + + |
| Bad | Good |
|---|---|
| + +```go +if data, err := os.ReadFile(name); err == nil { + err = cfg.Decode(data) + if err != nil { + return err + } + + fmt.Println(cfg) + return nil +} else { + return err +} +``` + + | + +```go +data, err := os.ReadFile(name) +if err != nil { + return err +} + +if err := cfg.Decode(data); err != nil { + return err +} + +fmt.Println(cfg) +return nil +``` + + |
| Bad | Good |
|---|---|
| + +```go +const ( + _defaultPort = 8080 + _defaultUser = "user" +) + +func Bar() { + fmt.Println("Default port", _defaultPort) +} +``` + + | + +```go +func Bar() { + const ( + defaultPort = 8080 + defaultUser = "user" + ) + fmt.Println("Default port", defaultPort) +} +``` + + |
| Bad | Good |
|---|
| Bad | Good |
|---|
| -``` +```plain failed to x: failed to y: failed to create new store: the error ``` |
-```
+```plain
x: y: new store: the error
```
@@ -1029,10 +962,7 @@ x: y: new store: the error
However once the error is sent to another system, it should be clear the
message is an error (e.g. an `err` tag or "Failed" prefix in logs).
-See also [Don't just check errors, handle them gracefully].
-
- [`"pkg/errors".Cause`]: https://godoc.org/github.com/pkg/errors#Cause
- [Don't just check errors, handle them gracefully]: https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully
+See also [Don't just check errors, handle them gracefully](https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully).
#### Error Naming
@@ -1087,13 +1017,123 @@ func (e *resolveError) Error() string {
}
```
+#### Handle Errors Once
+
+When a caller receives an error from a callee,
+it can handle it in a variety of different ways
+depending on what it knows about the error.
+
+These include, but not are limited to:
+
+- if the callee contract defines specific errors,
+ matching the error with `errors.Is` or `errors.As`
+ and handling the branches differently
+- if the error is recoverable,
+ logging the error and degrading gracefully
+- if the error represents a domain-specific failure condition,
+ returning a well-defined error
+- returning the error, either [wrapped](#error-wrapping) or verbatim
+
+Regardless of how the caller handles the error,
+it should typically handle each error only once.
+The caller should not, for example, log the error and then return it,
+because *its* callers may handle the error as well.
+
+For example, consider the following cases:
+
+
| |||||||||||||||||||||||||||||||||||
| -``` +```plain BenchmarkFmtSprint-4 143 ns/op 2 allocs/op ``` | -``` +```plain BenchmarkStrconv-4 64.2 ns/op 1 allocs/op ``` |
| Bad | Good |
|---|---|
| + +```go +func (c *client) request() { + caller := c.name + format := "json" + timeout := 5*time.Second + var err error + + // ... +} +``` + + | + +```go +func (c *client) request() { + var ( + caller = c.name + format = "json" + timeout = 5*time.Second + err error + ) + + // ... } ``` @@ -2269,20 +2610,15 @@ When naming packages, choose a name that is: - Not plural. For example, `net/url`, not `net/urls`. - Not "common", "util", "shared", or "lib". These are bad, uninformative names. -See also [Package Names] and [Style guideline for Go packages]. - - [Package Names]: https://blog.golang.org/package-names - [Style guideline for Go packages]: https://rakyll.org/style-packages/ +See also [Package Names](https://go.dev/blog/package-names) and [Style guideline for Go packages](https://rakyll.org/style-packages/). ### Function Names We follow the Go community's convention of using [MixedCaps for function -names]. An exception is made for test functions, which may contain underscores +names](https://go.dev/doc/effective_go#mixed-caps). An exception is made for test functions, which may contain underscores for the purpose of grouping related test cases, e.g., `TestMyFunction_WhatIsBeingTested`. - [MixedCaps for function names]: https://golang.org/doc/effective_go.html#mixed-caps - ### Import Aliasing Import aliasing must be used if the package name does not match the last @@ -2309,7 +2645,7 @@ direct conflict between imports. import ( "fmt" "os" - + runtimetrace "runtime/trace" nettrace "golang.net/x/trace" ) @@ -2511,8 +2847,6 @@ var _e error = F() Prefix unexported top-level `var`s and `const`s with `_` to make it clear when they are used that they are global symbols. -Exception: Unexported error values, which should be prefixed with `err`. - Rationale: Top-level variables and constants have a package scope. Using a generic name makes it easy to accidentally use the wrong value in a different file. @@ -2592,12 +2926,9 @@ type Client struct { Embedding should provide tangible benefit, like adding or augmenting functionality in a semantically-appropriate way. It should do this with zero -adverse user-facing effects (see also: [Avoid Embedding Types in Public Structs]). - -Exception: Mutexes should not be embedded, even on unexported types. See also: [Zero-value Mutexes are Valid]. +adverse user-facing effects (see also: [Avoid Embedding Types in Public Structs](#avoid-embedding-types-in-public-structs)). - [Avoid Embedding Types in Public Structs]: #avoid-embedding-types-in-public-structs - [Zero-value Mutexes are Valid]: #zero-value-mutexes-are-valid +Exception: Mutexes should not be embedded, even on unexported types. See also: [Zero-value Mutexes are Valid](#zero-value-mutexes-are-valid). Embedding **should not**: @@ -2743,9 +3074,7 @@ s := "foo" |
| Bad | Good |
|---|
| ```go -err := ioutil.WriteFile(name, data, 0644) +err := os.WriteFile(name, data, 0644) if err != nil { return err } @@ -2895,7 +3224,7 @@ if err != nil { | ```go -if err := ioutil.WriteFile(name, data, 0644); err != nil { +if err := os.WriteFile(name, data, 0644); err != nil { return err } ``` @@ -2912,7 +3241,7 @@ try to reduce the scope. |
| ```go -if data, err := ioutil.ReadFile(name); err == nil { +if data, err := os.ReadFile(name); err == nil { err = cfg.Decode(data) if err != nil { return err @@ -2928,7 +3257,7 @@ if data, err := ioutil.ReadFile(name); err == nil { | ```go -data, err := ioutil.ReadFile(name) +data, err := os.ReadFile(name) if err != nil { return err } @@ -2944,6 +3273,40 @@ return nil |
| Bad | Good |
|---|---|
| + +```go +const ( + _defaultPort = 8080 + _defaultUser = "user" +) + +func Bar() { + fmt.Println("Default port", _defaultPort) +} +``` + + | + +```go +func Bar() { + const ( + defaultPort = 8080 + defaultUser = "user" + ) + fmt.Println("Default port", defaultPort) +} +``` + + |
| Bad | Good |
|---|
| Bad | Good |
|---|
| Bad | Good |
|---|---|
| + +```go +func TestComplicatedTable(t *testing.T) { + tests := []struct { + give string + want string + wantErr error + shouldCallX bool + shouldCallY bool + giveXResponse string + giveXErr error + giveYResponse string + giveYErr error + }{ + // ... + } + + for _, tt := range tests { + t.Run(tt.give, func(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + if tt.shouldCallX { + xMock.EXPECT().Call().Return( + tt.giveXResponse, tt.giveXErr, + ) + } + yMock := ymock.NewMockY(ctrl) + if tt.shouldCallY { + yMock.EXPECT().Call().Return( + tt.giveYResponse, tt.giveYErr, + ) + } + + got, err := DoComplexThing(tt.give, xMock, yMock) + + // verify results + if tt.wantErr != nil { + require.EqualError(t, err, tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, want, got) + }) + } +} +``` + + | + +```go +func TestShouldCallX(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + xMock.EXPECT().Call().Return("XResponse", nil) + + yMock := ymock.NewMockY(ctrl) + + got, err := DoComplexThing("inputX", xMock, yMock) + + require.NoError(t, err) + assert.Equal(t, "want", got) +} + +func TestShouldCallYAndFail(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + + yMock := ymock.NewMockY(ctrl) + yMock.EXPECT().Call().Return("YResponse", nil) + + _, err := DoComplexThing("inputY", xMock, yMock) + assert.EqualError(t, err, "Y failed") +} +``` + |