Versions Compared

Key

  • This line was added.
  • This line was removed.
  • Formatting was changed.

...

For "internal" functions there is more flexibility, although the function name should reflect this (see e.g. log.closeFileLocked in pkg/util/log).

Bad

Good

Code Block
languagego
type Stats struct {
  sync.Mutex

  counters map[string]int
}

// Snapshot returns the current stats.
func (s *Stats) Snapshot() map[string]int {
  s.Lock()
  defer s.Unlock()

  return s.counters
}

// snapshot is no longer protected by the lock!
snapshot := stats.Snapshot()
Code Block
languagego
type Stats struct {
  sync.Mutex

  counters map[string]int
}

func (s *Stats) Snapshot() map[string]int {
  s.Lock()
  defer s.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()

...

Use defer to clean up resources such as files and locks.

Bad

Good

Code Block
languagego
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
Code Block
languagego
p.Lock()
defer p.Unlock()

if p.count < 10 {
  return p.count
}

p.count++
return p.count

// more readable

...

Bad

Good

Code Block
languagego
// Ought to be enough for anybody!
c := make(chan int, 64)
Code Block
languagego
// Size of one
c := make(chan int, 1) // or
// Unbuffered channel, size of zero
c := make(chan int)

...

The standard way of introducing enumerations in Go is to declare a custom type and a const group with iota. Since variables have a 0 default value, you should usually start your enums on a non-zero value.

Bad

Good

Code Block
languagego
type Operation int

const (
  Add Operation = iota
  Subtract
  Multiply
)

// Add=0, Subtract=1, Multiply=2
Code Block
languagego
type Operation int

const (
  Add Operation = iota + 1
  Subtract
  Multiply
)

// Add=1, Subtract=2, Multiply=3

There are cases where using the zero value makes sense, for example when the zero value case is the desirable default behavior.

Code Block
languagego
type LogOutput int

const (
  LogToStdout LogOutput = iota
  LogToFile
  LogToRemote
)

// LogToStdout=0, LogToFile=1, LogToRemote=2

...

Bad

Good

Code Block
languagego
for i := 0; i < b.N; i++ {
  s := fmt.Sprint(rand.Int())
}
Code Block
languagego
for i := 0; i < b.N; i++ {
  s := strconv.Itoa(rand.Int())
}
Code Block
BenchmarkFmtSprint-4    143 ns/op    2 allocs/op
Code Block
BenchmarkStrconv-4    64.2 ns/op    1 allocs/op

...

Do not create byte slices from a fixed string repeatedly: it causes a heap allocation and a copy. Instead, perform the conversion once and capture the result.

Bad

Good

Code Block
languagego
for i := 0; i < b.N; i++ {
  w.Write([]byte("Hello world"))
}
Code Block
languagego
data := []byte("Hello world")
for i := 0; i < b.N; i++ {
  w.Write(data)
}
Code Block
BenchmarkBad-4   50000000   22.2 ns/op
Code Block
BenchmarkGood-4  500000000   3.25 ns/op

...


If the arguments list is too long to fit on a single line, switch to one argument per line:

Code Block
languagego
func (s *someType) myFunctionName(
   arg1 somepackage.SomeArgType,
   arg2 int,
   arg3 somepackage.SomeOtherType,
) (somepackage.SomeReturnType, error) {
...
}

If the return types need to be wrapped, use the same rules:

Code Block
languagego
func (s *someType) myFunctionName(
  arg1 somepackage.SomeArgType, arg2 somepackage.SomeOtherType,
) (
  somepackage.SomeReturnType,
  somepackage.SomeOtherType,
  error,
) {
...
}

...

Go supports grouping similar declarations.

Bad

Good

Code Block
languagego
import "a"
import "b"
Code Block
languagego
import (
  "a"
  "b"
)

This also applies to constants, variables, and type declarations.

Bad

Good

Code Block
languagego
const a = 1
const b = 2



var a = 1
var b = 2



type Area float64
type Volume float64
Code Block
languagego
const (
  a = 1
  b = 2
)

var (
  a = 1
  b = 2
)

type (
  Area float64
  Volume float64
)

Only group related declarations. Do not group declarations that are unrelated.

Bad

Good

Code Block
languagego
type Operation int

const (
  Add Operation = iota + 1
  Subtract
  Multiply
  ENV_VAR = "MY_ENV"
)
Code Block
languagego
type Operation int

const (
  Add Operation = iota + 1
  Subtract
  Multiply
)

const ENV_VAR = "MY_ENV"

Groups are not limited in where they can be used. For example, you can use them inside of functions.

Bad

Good

Code Block
languagego
func f() string {
  var red = color.New(0xff0000)
  var green = color.New(0x00ff00)
  var blue = color.New(0x0000ff)

  ...
}
Code Block
languagego
func f() string {
  var (
    red   = color.New(0xff0000)
    green = color.New(0x00ff00)
    blue  = color.New(0x0000ff)
  )

  ...
}

...

This is the grouping applied by goimports by default.

Bad

Good

Code Block
languagego
import (
  "fmt"
  "os"
  "go.uber.org/atomic"
  "golang.org/x/sync/errgroup"
)
Code Block
languagego
import (
  "fmt"
  "os"

  "go.uber.org/atomic"
  "golang.org/x/sync/errgroup"
)

...

Import aliasing must be used if the package name does not match the last element of the import path.

Code Block
languagego
import (
  "net/http"

  client "example.com/client-go"
  trace "example.com/trace/v2"
)

In all other scenarios, import aliases should be avoided unless there is a direct conflict between imports.

Bad

Good

Code Block
languagego
import (
  "fmt"
  "os"


  nettrace "golang.net/x/trace"
)
Code Block
languagego
import (
  "fmt"
  "os"
  "runtime/trace"

  nettrace "golang.net/x/trace"
)

...

Since functions are grouped by receiver, plain utility functions should appear towards the end of the file.

Bad

Good

Code Block
languagego
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{}
}
Code Block
languagego
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 {...}

...

Code should reduce nesting where possible by handling error cases/special conditions first and returning early or continuing the loop. Reduce the amount of code that is nested multiple levels.

Bad

Good

Code Block
languagego
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)
  }
}
Code Block
languagego
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()
}

...

If a variable is set in both branches of an if, it can be replaced with a single if:

Bad

Good

Code Block
languagego
var a int
if b {
  a = 100
} else {
  a = 10
}
Code Block
languagego
a := 10
if b {
  a = 100
}

A return statement inside of an if block means that you won't need an "else" clause.

Bad

Good

Code Block
languagego
if b {
  return "yup"
} else {
  return "nope"
}
Code Block
languagego
if b {
  return "yup"
}
return "nope"

...

At the top level, use the standard var keyword. Do not specify the type, unless it is not the same type as the expression.

Bad

Good

Code Block
languagego
var _s string = F()

func F() string { return "A" }
Code Block
languagego
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" }

Specify the type if the type of the expression does not match the desired type exactly.

Code Block
languagego
type myError struct{}

func (myError) Error() string { return "error" }

func F() myError { return myError{} }

var _e error = F()
// F returns an object of type myError but we want error.

...

You should almost always specify field names when initializing structs. This is now enforced by go vet.

Bad

Good

Code Block
languagego
k := User{"John", "Doe", true}
Code Block
languagego
k := User{
    FirstName: "John",
    LastName: "Doe",
    Admin: true,
}

Exception: Field names may be omitted in test tables when there are 3 or fewer fields.

Code Block
languagego
tests := []struct{
}{
  op Operation
  want string
}{
  {Add, "add"},
  {Subtract, "subtract"},
}

...

Short variable declarations (:=) should be used if a variable is being set to some value explicitly.

Bad

Good

Code Block
languagego
var s = "foo"
Code Block
languagego
s := "foo"

However, there are cases where the default value is clearer when the var keyword is use. Declaring Empty Slices, for example.

Bad

Good

code
Code Block
language
go
func f(list []int) {
  filtered := []int{}
  for _, v := range list {
    if v > 10 {
      filtered = append(filtered, v)
    }
  }
}
Code Block
languagego
func f(list []int) {
  var filtered []int
  for _, v := range list {
    if v > 10 {
      filtered = append(filtered, v)
    }
  }
}

...

To check if a slice is empty, always use len(s) == 0. Do not check for nil.

Bad

Good

Code Block
languagego
func isEmpty(s []string) bool {
 return s == nil
}
Code Block
languagego
func isEmpty(s []string) bool {
 return len(s) == 0
}

The zero value (a slice declared with var) is usable immediately without make().

Bad

Good

Code Block
languagego
nums := []int{}
// or nums := make([]int)

if add1 {
  nums = append(nums, 1)
}

if add2 {
  nums = append(nums, 2)
}
Code Block
languagego
var nums []int


if add1 {
  nums = append(nums, 1)
}

if add2 {
  nums = append(nums, 2)
}

...

Where possible, reduce scope of variables. Do not reduce the scope if it conflicts with Reduce Nesting.

Bad

Good

Code Block
languagego
err := f.Close()
if err != nil {
 return err
}
Code Block
languagego
if err := f.Close(); err != nil {
 return err
}

If you need a result of a function call outside of the if, then you should not try to reduce the scope.

Bad

Good

Code Block
languagego
if f, err := os.Open("f"); err == nil {
  _, err = io.WriteString(f, "data")
  if err != nil {
    return err
  }
  return f.Close()
} else {
  return err
}
Code Block
languagego
f, err := os.Open("f")
if err != nil {
   return err
}

if _, err := io.WriteString(f, "data"); err != nil {
  return err
}

return f.Close()

...

Naked parameters in function calls can hurt readability. Add C-style comments (/* ... */) for parameter names when their meaning is not obvious.

Bad

Good

Code Block
languagego
// func printInfo(name string, isLocal, done bool)

printInfo("foo", true, true)
Code Block
languagego
// func printInfo(name string, isLocal, done bool)

printInfo("foo", true /* isLocal */, true /* done */)

Better yet, replace naked bool types with custom types for more readable and type-safe code. This allows more than just two states (true/false) for that parameter in the future.

Code Block
languagego
type Region int

const (
  UnknownRegion Region = iota
  Local
)

type Status int

const (
  StatusReady = iota + 1
  StatusDone
  // Maybe we will have a StatusInProgress in the future.
)

func printInfo(name string, region Region, status Status)

...

bool arguments to functions are often dubious things, as they hint to code that essentially reads like:

Code Block
languagego
func doSomething(shouldDoX bool) {
  if shouldDoX {
    doX()
  } else {
    doY()
  }
}

...

In cases where the `bool` in question, along with other arguments, acts as a "knob" to the function consider replacing it with some type of "configuration" struct (for examples, see [Dave Cheney's treatment of the topic](https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis)). In situations where there's a single bool param or the situation is less clear-cut, consider replacing the `bool` in question with an enum. For example:

Code Block
languagego
type EndTxnAction bool

const (
Commit EndTxnAction = false
Abort = true
)

func endTxn(action EndTxnAction) {}

...

Go supports raw string literals, which can span multiple lines and include quotes. Use these to avoid hand-escaped strings which are much harder to read.

Bad

Good

Code Block
languagego
wantError := "unknown name:\"test\""
Code Block
languagego
wantError := `unknown error:"test"`

...

Use &T{} instead of new(T) when initializing struct references so that it is consistent with the struct initialization.

Bad

Good

Code Block
languagego
sval := T{Name: "foo"}

// inconsistent
sptr := new(T)
sptr.Name = "bar"
Code Block
languagego
sval := T{Name: "foo"}

sptr := &T{Name: "bar"}

...

This helps go vet perform static analysis of the format string.

Bad

Good

Code Block
languagego
msg := "unexpected values %v, %v\n"
fmt.Printf(msg, 1, 2)
Code Block
languagego
const msg = "unexpected values %v, %v\n"
fmt.Printf(msg, 1, 2)

...

Use table-driven tests with subtests to avoid duplicating code when the core test logic is repetitive.

Bad

Good

Code Block
languagego
// 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)
Code Block
languagego
// 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)
  })
}

...

We follow the convention that the slice of structs is referred to as tests and each test case tt. Further, we encourage explicating the input and output values for each test case with give and want prefixes.

Code Block
languagego
tests := []struct{
  give     string
  wantHost string
  wantPort string
}{
  // ...
}

for _, tt := range tests {
  // ...
}

...

Use this pattern for optional arguments in constructors and other public APIs that you foresee needing to expand, especially if you already have three or more arguments on those functions.

Bad

Good

Code Block
languagego
// package db

func Connect(
  addr string,
  timeout time.Duration,
  caching bool,
) (*Connection, error) {
  // ...
}

// Timeout and caching must always be provided,
// even if the user wants to use the default.

db.Connect(addr, db.DefaultTimeout, db.DefaultCaching)
db.Connect(addr, newTimeout, db.DefaultCaching)
db.Connect(addr, db.DefaultTimeout, false /* caching */)
db.Connect(addr, newTimeout, false /* caching */)
Code Block
languagego
type options struct {
  timeout time.Duration
  caching bool
}

// Option overrides behavior of Connect.
type Option interface {
  apply(*options)
}

type optionFunc func(*options)

func (f optionFunc) apply(o *options) {
  f(o)
}

func WithTimeout(t time.Duration) Option {
  return optionFunc(func(o *options) {
    o.timeout = t
  })
}

func WithCaching(cache bool) Option {
  return optionFunc(func(o *options) {
    o.caching = cache
  })
}

// Connect creates a connection.
func Connect(
  addr string,
  opts ...Option,
) (*Connection, error) {
  options := options{
    timeout: defaultTimeout,
    caching: defaultCaching,
  }

  for _, o := range opts {
    o.apply(&options)
  }

  // ...
}

// Options must be provided only if needed.

db.Connect(addr)
db.Connect(addr, db.WithTimeout(newTimeout))
db.Connect(addr, db.WithCaching(false))
db.Connect(
  addr,
  db.WithCaching(false),
  db.WithTimeout(newTimeout),
)

...