Skip to end of metadata
Go to start of metadata

You are viewing an old version of this page. View the current version.

Compare with Current View Page History

« Previous Version 27 Current »

This coding guide is inspired from the Uber Go style guide at https://github.com/uber-go/guide/blob/master/style.md, with modifications specific to Cockroach Labs.

Introduction

These guidelines are the conventions that govern our code. These conventions cover far more than just source file formatting—gofmt handles that for us.

The goal of this guide is to manage this complexity by describing in detail the Dos and Don'ts of writing Go code at Cockroach Labs. These rules exist to keep the code base manageable while still allowing engineers to use Go language features productively.

This documents idiomatic conventions in Go code that we follow at CRL. A lot of these are general guidelines for Go, while others extend upon external resources:

  1. Effective Go

  2. The Go common mistakes guide

All code should be error-free when run through golint and go vet. We recommend setting up your editor to:

  • Run crlfmt on save

  • Run make lintshort to check for errors

You can find information in editor support for Go tools here: golang/go/wiki/IDEsAndTextEditorPlugins

Guidelines

Pointers to Interfaces

You almost never need a pointer to an interface. You should be passing interfaces as values—the underlying data can still be a pointer.

An interface is two fields:

  1. A pointer to some type-specific information. You can think of this as "type."

  2. Data pointer. If the data stored is a pointer, it’s stored directly. If the data stored is a value, then a pointer to the value is stored.

If you want interface methods to modify the underlying data, you must use a pointer.

Receivers and Interfaces

Methods with value receivers can be called on pointers as well as values.

For example,

type S struct {
  data string
}

func (s S) Read() string {
  return s.data
}

func (s *S) Write(str string) {
  s.data = str
}

sVals := map[int]S{1: {"A"}}

// With a value you can call read, but not Write.
sVals[1].Read()

// This will not compile:
//  sVals[0].Write("test")

sPtrs := map[int]*S{1: {"A"}}

// You can call both Read and Write using a pointer
sPtrs[1].Read()
sPtrs[1].Write("test")

Similarly, an interface can be satisfied by a pointer, even if the method has a value receiver.

type F interface {
  f()
}

type S1 struct{}

func (s S1) f() {}

type S2 struct{}

func (s *S2) f() {}

s1Val := S1{}
s1Ptr := &S1{}
s2Val := S2{}
s2Ptr := &S2{}

var i F
i = s1Val
i = s1Ptr
i = s2Ptr

// The following doesn't compile, since s2Val is a value, and there is no value receiver for f.
//   i = s2Val

Effective Go has a good write up on Pointers vs. Values.

Zero-value Mutexes are Valid

The zero-value of sync.Mutex and sync.RWMutex is valid, so you almost never need a pointer to a mutex.

Bad

Good

mu := new(sync.Mutex)
mu.Lock()
var mu sync.Mutex
mu.Lock()

If you use a struct by pointer, then the mutex can be a non-pointer field or, preferably, embedded directly into the struct.

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]
}
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]
}

Embed for private types or types that need to implement the Mutex interface.

For exported types, use a private lock.

Copy Slices and Maps when necessary

Slices and maps contain pointers to the underlying data so be conscious of ownership when passing them as parameters or returning them as values.

On the other hand, do not unnecessarily copy slices - heap allocations are expensive and we want to avoid them when possible.

Receiving Slices and Maps

Keep in mind that users can modify a map or slice you received as an argument if you store a reference to it.

If your API captures a slice by reference, document it in the function header.

Bad

Good

func (d *Driver) SetTrips(trips []Trip) {
  d.trips = trips
}

trips := ...
d1.SetTrips(trips)

// Did you mean to modify d1.trips?
trips[0] = ...
// SetTrips sets the driver's trips.
// Note that the slice is captured by reference, the
// caller should take care of prevening unwanted aliasing.
func (d *Driver) SetTrips(trips []Trip) { d.trips = trips }

// or:

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] = ...

Returning Slices and Maps

Similarly, be wary of user modifications to maps or slices exposing internal state.

Your exported API should refrain from returning internal state that otherwise has accessors, e.g. when it is hidden behind a mutex.

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

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()
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()

Defer to Clean Up

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

Bad

Good

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

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

p.count++
return p.count

// more readable

Defer has an extremely small overhead and should be avoided only if you can prove that your function execution time is in the order of nanoseconds. The readability win of using defers is worth the minuscule cost of using them. This is especially true for larger methods that have more than simple memory accesses, where the other computations are more significant than the defer

Sometimes, you may wish to Lock a mutex, do some work, and the Unlock it before the end of the function has been reached. If the work being done in this critical section has the potential to panic, this could end up leaving the mutex locked if the executing code is wrapped in a panic handler, leading to deadlocks.

To protect against this, execute the code in a separate function so that defer can still be used. This way, even if a panic occurs in the critical section, the mutex will be released.

Bad

Good

func myFunc() error {
  // do some work...
  p.Lock()
  if err := somethingThatCanPanic(); err != nil {
    p.Unlock()
    return err
  }
  p.Unlock()
  // do some more work...
  return nil
}

// if somethingThatCanPanic() panics, the mutex will be left locked.
func myFunc() error {
  // do some work...
  doWork := func() error {
    p.Lock()
    defer p.Unlock()
    return somethingThatCanPanic()
  }
  if err := doWork(); err != nil {
    return err
  }
  // do some more work...
  return nil
}

// enables us to continue use defer without leaving the mutex locked longer than necessary.

Channel Size is One or None

Channels should usually have a size of one or be unbuffered. By default, channels are unbuffered and have a size of zero. Any other size must be subject to a high level of scrutiny. Consider how the size is determined, what prevents the channel from filling up under load and blocking writers, and what happens when this occurs.

Bad

Good

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

Start Enums at One

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

type Operation int

const (
  Add Operation = iota
  Subtract
  Multiply
)

// Add=0, Subtract=1, Multiply=2
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.

type LogOutput int

const (
  LogToStdout LogOutput = iota
  LogToFile
  LogToRemote
)

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

Error handling

The following sub-section are an excerpt from https://cockroachlabs.atlassian.net/wiki/pages/resumedraft.action?draftId=1676804870 . Read the full page for more background and context.

Error Types

We prefer the use of the CockroachDB errors library at github.com/cockroachdb/errors. This is a superset of Go's own errors and pkg/errors.

  • errors.New() for errors with simple static strings

  • errors.Newf() for formatted error strings

  • errors.AssertionFailedf()  for assertion / internal errors that reflect a likely implementation bug by the CRL team and should be louder than usual

  • Add context on return paths using errors.Wrap() / errors.Wrapf()

  • Add user-directed hints with errors.WithHint()

  • See the doc README on the errors package for more

When returning errors, consider the following to determine the best choice:

  • Is this a simple error that needs no extra information? If so, errors.New should suffice.

  • Do the clients need to detect and handle this error? If so, you should use a custom type, and implement the Error() method.

  • Are you propagating an error returned by a downstream function? If so, check the RFC on error handling

If the client needs to detect the error, and you have created a simple error using errors.New, use a var for the error.

Bad

Good

// package foo

func Open() error {
  return errors.New("could not open")
}

// package bar

  if err := foo.Open(); err != nil {
    if err.Error() == "could not open" {
      // handle
    } else {
      panic("unknown error")
    }
  }
// 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
  } else {
    return errors.Wrap(err, "unknown error")
  }
}

If you have an error that clients may need to detect, and you would like to add more information to it (e.g., it is not a static string), then you should use a custom type.

Bad

Good

func open(file string) error {
  return fmt.Errorf("file %q not found", file)
}

func use() error {
  if err := open(); err != nil {
    if strings.Contains(err.Error(), "not found") {
      // handle
    } else {
      panic("unknown error")
    }
  }
}
type errNotFound struct {
  file string
}

func (e errNotFound) Error() string {
  return fmt.Sprintf("file %q not found", e.file)
}

func open(file string) error {
  return errNotFound{file: file}
}

func use() error {
  if err := open(); err != nil {
    var nfErr errNotFound
    if errors.As(err, &nfErr) {
      // handle
    } else {
      return errors.Wrap(err, "opening file")
    }
  }
}

Error Wrapping

There are three main options for propagating errors if a call fails:

  • Return the original error if there is no additional context to add and you want to maintain the original error type.

  • Add context using errors.Wrap so that the error message provides more context and errors.Unwrap can be used to extract the original error. Prefer this to fmt.Errorf("some message %w").

  • If you pass an error through a channel from one goroutine to another, use errors.WithStack on both ends to identify this flow.

  • Use errors.Newf if the callers do not need to detect or handle that specific error case.

  • Use errors.Handle or errors.NewAssertionErrorWithWrappedErrf to hide the original cause.

It is recommended to add context where possible so that instead of a vague error such as "connection refused", you get more useful errors such as "call service foo: connection refused".

When adding context to returned errors, keep the context succinct by avoiding phrases like "failed to", which state the obvious and pile up as the error percolates up through the stack:

Bad

Good

s, err := store.New()
if err != nil {
    return errors.Newf(
        "failed to create new store: %s", err)
}
s, err := store.New()
if err != nil {
    return errors.Wrap(
        "new store", err)
}
failed to x: failed to y: failed to create new store: the error
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 — but be mindful that Go has evolved since this article was written, and use errors.Is() and errors.As() where applicable.

Handle Type Assertion Failures

The single return value form of a type assertion will panic on an incorrect type. Therefore, always use the "comma ok" idiom.

Bad

Good

t := i.(string)
t, ok := i.(string)
if !ok {
  // handle the error gracefully
}

Don't Panic

Code running in production must avoid panics. Panics are a major source of cascading failures. If an error occurs, the function must return an error and allow the caller to decide how to handle it.

Bad

Good

func foo(bar string) {
  if len(bar) == 0 {
    panic("bar must not be empty")
  }
  // ...
}

func main() {
  if len(os.Args) != 2 {
    fmt.Println("USAGE: foo <bar>")
    os.Exit(1)
  }
  foo(os.Args[1])
}
func foo(bar string) error {
  if len(bar) == 0
    return errors.New("bar must not be empty")
  }
  // ...
  return nil
}

func main() {
  if len(os.Args) != 2 {
    fmt.Println("USAGE: foo <bar>")
    os.Exit(1)
  }
  if err := foo(os.Args[1]); err != nil {
    panic(err)
  }
}

Panic/recover is, in most cases, not an error handling strategy. A program must panic only when something irrecoverable happens such as a nil dereference.

An exception to this is program initialization: bad things at program startup that should abort the program may cause panic.

var _statusTemplate = template.Must(template.New("name").Parse("_statusHTML"))

Another exception is when a package is purely functional and has no side effects. For example, parts of SQL planning in CockroachDB uses panic propagation for error handling in that case.

Even in tests, prefer t.Fatal or t.FailNow over panics to ensure that the test is marked as failed.

Bad

Good

// func TestFoo(t *testing.T)

f, err := ioutil.TempFile("", "test")
if err != nil {
  panic("failed to set up test")
}
// func TestFoo(t *testing.T)

f, err := ioutil.TempFile("", "test")
if err != nil {
  t.Fatal("failed to set up test")
}

Performance

Performance-specific guidelines apply only to the hot path.

Be mindful that strconv is faster than fmt

When converting primitives to/from strings, strconv is faster than fmt.

Bad

Good

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

Avoid string-to-byte conversion

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

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

Style

Code comments

See https://wiki.crdb.io/wiki/spaces/CRDB/pages/181371303/Go+Golang+coding+guidelines

Line Length

Format your code assuming it will be read in a window 100 columns wide. Wrap code at 100 characters and comments at 80 unless doing so makes the code less legible. These values assume tab width is 2 characters.

Wrapping Function Signatures

When wrapping function signatures that do not fit on one line, put the name, arguments, and return types on separate lines, with the closing ) at the same indentation as func (this helps visually separate the indented arguments from the indented function body). Example:

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


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

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:

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

Exception when omitting repeated types for consecutive arguments: short and related arguments (e.g. `start, end int64`) should either go on the same line or the type should be repeated on each line -- no argument should appear by itself on a line with no type  (confusing and brittle when edited).

Group Similar Declarations

Go supports grouping similar declarations.

Bad

Good

import "a"
import "b"
import (
  "a"
  "b"
)

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

Bad

Good

const a = 1
const b = 2



var a = 1
var b = 2



type Area float64
type Volume float64
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

type Operation int

const (
  Add Operation = iota + 1
  Subtract
  Multiply
  ENV_VAR = "MY_ENV"
)
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

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

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

  ...
}

Import Group Ordering

There should be two import groups:

  • Standard library

  • Everything else

This is the grouping applied by goimports by default.

Bad

Good

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

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

Package Names

When creating and naming packages, refer to the separate page Organization of code into 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 for the purpose of grouping related test cases, e.g., TestMyFunction_WhatIsBeingTested.

Import Aliasing

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

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

import (
  "fmt"
  "os"


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

  nettrace "golang.net/x/trace"
)

Function Grouping and Ordering

  • Functions should be sorted in rough call order.

  • Functions in a file should be grouped by receiver.

Therefore, exported functions should appear first in a file, after struct, const, var definitions.

A newXYZ()/NewXYZ() may appear after the type is defined, but before the rest of the methods on the receiver.

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

Bad

Good

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{}
}
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 {...}

Reduce Nesting

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

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)
  }
}
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()
}

Unnecessary Else

Avoid "else" clauses if they're not needed.

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

Bad

Good

var a int
if b {
  a = 100
} else {
  a = 10
}
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

if b {
  return "yup"
} else {
  return "nope"
}
if b {
  return "yup"
}
return "nope"

Top-level Variable Declarations

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

var _s string = F()

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

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, an interface.

Use Field Names to initialize Structs

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

Bad

Good

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

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

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

Local Variable Declarations

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

Bad

Good

var s = "foo"
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

func f(list []int) {
  filtered := []int{}
  for _, v := range list {
    if v > 10 {
      filtered = append(filtered, v)
    }
  }
}
func f(list []int) {
  var filtered []int
  for _, v := range list {
    if v > 10 {
      filtered = append(filtered, v)
    }
  }
}

nil is a valid slice

nil is a valid slice of length 0. This means that,

You should not return a slice of length zero explicitly. Return nil instead.

Bad

Good

return []int{}
return nil

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

Bad

Good

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

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

Bad

Good

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

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

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


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

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

Reduce Scope of Variables

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

Bad

Good

err := f.Close()
if err != nil {
 return err
}
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

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

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

return f.Close()

Avoid Naked Parameters

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

Bad

Good

// func printInfo(name string, isLocal, done bool)

printInfo("foo", true, true)
// 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.

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)

Note: For `bool` constants, like for all literals, the comment should indicate the name of the parameter and does not depend on the argument value. *Do not* put a bang in the comment when commenting the `false` constant. Also, do not adapt the comment to negate the name of the parameter. For example:

func endTxn(commit bool){}
OK:     endTxn(false /* commit */)
NOT OK: endTxn(false /* !commit */)
NOT OK: endTxn(false /* abort */)
// If you want to add an explanation to an argument, a suggested style is to
// include both the param name and the explanation with a dash between them:
OK: endTxn(false /* commit - we abort as we concluded above that we can't commit */)

Try to avoid bool parameters

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

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


This is not always the case. However, in cases where that is a fair assessment of the situation, consider whether the doSomething function should exist at all.

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:

type EndTxnAction bool

const (
Commit EndTxnAction = false
Abort = true
)

func endTxn(action EndTxnAction) {}

is better than:

func endTxn(commit bool) {}

fmt Verbs

Prefer the most specific verb for your use. In other words, prefer to avoid %v when possible. However, %v is to be used when formatting bindings which might be nil and which do not already handle nil formatting. Notably, nil errors formatted as %s will render as %!s(<nil>) while nil errors formatted as %v will render as <nil>. Therefore, prefer %v when formatting errors which are not known to be non-nil.

Use Raw String Literals to Avoid Escaping

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

wantError := "unknown name:\"test\""
wantError := `unknown error:"test"`

Initializing Struct References

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

Bad

Good

sval := T{Name: "foo"}

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

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

Format Strings outside Printf

If you declare format strings for Printf-style functions outside a string literal, make them const values.

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

Bad

Good

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

Naming Printf-style Functions

When you declare a Printf-style function, make sure that go vet can detect it and check the format string.

This means that you should use pre-defined Printf-style function names if possible. go vet will check these by default. See Printf family for more information.

If using the pre-defined names is not an option, end the name you choose with f: Wrapf, not Wrap. go vet can be asked to check specific Printf-style names but they must end with f.

$ go vet -printfuncs=wrapf,statusf

See also go vet: Printf family check.

Go implementation of SQL

When defining the result column labels of statements or virtual tables, apply the following general principles:

  • the casing of column labels must be consistent.

  • the same concept in different statements should be named using the same word.
    For example, `variable` and `value` corresponds between SHOW ALL CLUSTER SETTINGS and SHOW SESSION ALL.

  • the labels must be usable by a surrounding query without mandating quotes.
    For example, `start_key` instead of `"Start Key"`. Also, avoid SQL keywords which would also need to be quoted: `table_name` instead of `"table"`, `index_name` instead of `"index"`, etc.
    Never name a column `"user"`! The risk of mistake with the syntax special form `user` is too high. Use `user_name` instead.

  • use underscores to separate words, for consistency with `information_schema`.
    For example, `start_key` instead of `startkey`

  • avoid abbreviations unless most uses of the word in spoken English use the abbreviation already.
    For example, `id` for "identifier" is OK, but `loc` for "location" is not.

  • for columns akin to "primary keys" (that identify the object being described by the remaining columns), words whose meaning is highly overloaded must be disambiguated in the label.
    For example, `zone_id`, `job_id`, `table_id` instead of just `id` everywhere. `table_name`, `column_name`, etc instead of `name` everywhere.

  • for database objects that can be referred to by two or more types of handles (e.g. by-ID or by-name), the column label must disambiguate which type of handle is being reported. This makes it possible to later report the other handles in additional columns.
    For example, `table_name` instead of `table`, so that `table_id` can be added later.

  • labels that reproduce data already present in an `information_schema` table or a prior `SHOW` statement should use the same labels as the `information_schema` table or `SHOW` statement, provided it matches the principles above already.


Patterns

Data-driven tests

We provide an alternative to table-driven tests (see next section) in CockroachDB via the datadriven test package.

package datadriven

// RunTest invokes a data-driven test. The test cases are contained in a
// separate test file and are dynamically loaded, parsed, and executed by this
// testing framework. By convention, test files are typically located in a
// sub-directory called "testdata". Each test file has the following format:
//
//   <command>[,<command>...] [arg | arg=val | arg=(val1, val2, ...)]...
//   <input to the command>
//   ----
//   <expected results>
//
// The command input can contain blank lines. However, by default, the expected
// results cannot contain blank lines. This alternate syntax allows the use of
// blank lines:
//
//   <command>[,<command>...] [arg | arg=val | arg=(val1, val2, ...)]...
//   <input to the command>
//   ----
//   ----
//   <expected results>
//
//   <more expected results>
//   ----
//   ----
//
// To execute data-driven tests, pass the path of the test file as well as a
// function which can interpret and execute whatever commands are present in
// the test file. The framework invokes the function, passing it information
// about the test case in a TestData struct. The function then returns the
// actual results of the case, which this function compares with the expected
// results, and either succeeds or fails the test.
func RunTest(t *testing.T, path string, f func(d *TestData) string) 


Test Tables

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

Bad

Good

// 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)
// 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)
  })
}

Test tables make it easier to add context to error messages, reduce duplicate logic, and add new test cases.

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.

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

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

Functional Options

Functional options is a pattern in which you declare an opaque Option type that records information in some internal struct. You accept a variadic number of these options and act upon the full information recorded by the options on the internal struct.

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

// 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 */)
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),
)

See also about functional options:

  • No labels