From b5ab857880f63cac7ce213e5dd5be8036f70d3c4 Mon Sep 17 00:00:00 2001 From: TangoEnSkai <21152231+TangoEnSkai@users.noreply.github.com> Date: Thu, 17 Oct 2019 03:25:30 +0900 Subject: [PATCH] finish translate for introduction --- README.md | 2192 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 2191 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index a828ac0..169edb0 100644 --- a/README.md +++ b/README.md @@ -1 +1,2191 @@ -# uber-go-style-guide-kr \ No newline at end of file +# uber-go-style-guide-kr + +Translated in Korean + +--- + + + +# Uber의 Go언어 스타일 가이드 + +## 목차 + +- [uber-go-style-guide-kr](#uber-go-style-guide-kr) +- [Uber의 Go언어 스타일 가이드](#uber%ec%9d%98-go%ec%96%b8%ec%96%b4-%ec%8a%a4%ed%83%80%ec%9d%bc-%ea%b0%80%ec%9d%b4%eb%93%9c) + - [목차](#%eb%aa%a9%ec%b0%a8) + - [Introduction](#introduction) + - [Guidelines](#guidelines) + - [Pointers to Interfaces](#pointers-to-interfaces) + - [Receivers and Interfaces](#receivers-and-interfaces) + - [Zero-value Mutexes are Valid](#zero-value-mutexes-are-valid) + - [Copy Slices and Maps at Boundaries](#copy-slices-and-maps-at-boundaries) + - [Receiving Slices and Maps](#receiving-slices-and-maps) + - [Returning Slices and Maps](#returning-slices-and-maps) + - [Defer to Clean Up](#defer-to-clean-up) + - [Channel Size is One or None](#channel-size-is-one-or-none) + - [Start Enums at One](#start-enums-at-one) + - [Error Types](#error-types) + - [Error Wrapping](#error-wrapping) + - [Handle Type Assertion Failures](#handle-type-assertion-failures) + - [Don't Panic](#dont-panic) + - [Use go.uber.org/atomic](#use-gouberorgatomic) + - [Performance](#performance) + - [Prefer strconv over fmt](#prefer-strconv-over-fmt) + - [Avoid string-to-byte conversion](#avoid-string-to-byte-conversion) + - [Style](#style) + - [Group Similar Declarations](#group-similar-declarations) + - [Import Group Ordering](#import-group-ordering) + - [Package Names](#package-names) + - [Function Names](#function-names) + - [Import Aliasing](#import-aliasing) + - [Function Grouping and Ordering](#function-grouping-and-ordering) + - [Reduce Nesting](#reduce-nesting) + - [Unnecessary Else](#unnecessary-else) + - [Top-level Variable Declarations](#top-level-variable-declarations) + - [Prefix Unexported Globals with _](#prefix-unexported-globals-with) + - [Embedding in Structs](#embedding-in-structs) + - [Use Field Names to initialize Structs](#use-field-names-to-initialize-structs) + - [Local Variable Declarations](#local-variable-declarations) + - [nil is a valid slice](#nil-is-a-valid-slice) + - [Reduce Scope of Variables](#reduce-scope-of-variables) + - [Avoid Naked Parameters](#avoid-naked-parameters) + - [Use Raw String Literals to Avoid Escaping](#use-raw-string-literals-to-avoid-escaping) + - [Initializing Struct References](#initializing-struct-references) + - [Format Strings outside Printf](#format-strings-outside-printf) + - [Naming Printf-style Functions](#naming-printf-style-functions) + - [Patterns](#patterns) + - [Test Tables](#test-tables) + - [Functional Options](#functional-options) + +## Introduction + +스타일은 코드를 통제하는(govern) 관습이다. 이러한 관습(convention)은 소스파일 포맷팅 (e.g. gofmt)보다 더 많은 영역을 다루기(cover) 때문에, "스타일" 이라는 단어 자체가 약간 부적절 할 수 있다. + +본 가이드의 목표는 Uber에서 Go 코드를 작성할 때 해야 할 것과 하지 말아야 할 것 (Dos and Don'ts)에 대하여 자세하게 설명하여 이러한 복잡성을 관리하는 것이다. 이런 규칙들은 엔지니어들이 Go 언어의 특성을(feature) 생산적으로 계속 사용할 수 있도록 코드 베이스를 관리가능하게 유지하기위해 존재한다. + +이 가이드는 원래 [Prashant Varanasi] [Simon Newton]as +a way to bring some colleagues up to speed with using Go. Over the years it has +been amended based on feedback from others. + + [Prashant Varanasi]: https://github.com/prashantv + [Simon Newton]: https://github.com/nomis52 + +이 문서는 Uber에서의 엔지니어들이 지향하는 Go언어 코드의 관용적 규칙을 설명한다. 상당 수의 규칙들은 Go언어에 대한 일반적인 가이드라인이며, 다른 부분에 대해서는 외부 레퍼런스에 의해 확장된다 (아래 참고) + +1. [Effective Go](https://golang.org/doc/effective_go.html) +2. [The Go common mistakes guide](https://github.com/golang/go/wiki/CodeReviewComments) + +모든 코드는 `golint` 와 `go vet`를 실행할 때 에러가 없어야 한다. 또한 우리는 여러분들의 에디터를 아래와 같이 설정하기를 권고한다: + +- Run `goimports` on save +- Run `golint` and `go vet` to check for errors + +아래의 링크를 통해서 Go 툴을 지원하는 에디터에 대한 정보를 얻을 수 있다: + + +## 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, + +```go +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"}} + +// You can only call Read using a value +sVals[1].Read() + +// This will not compile: +// sVals[1].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. + +```go +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]. + + [Pointers vs. Values]: https://golang.org/doc/effective_go.html#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. + + + + + +
BadGood
+ +```go +mu := new(sync.Mutex) +mu.Lock() +``` + + + +```go +var mu sync.Mutex +mu.Lock() +``` + +
+ +If you use a struct by pointer, then the mutex can be a non-pointer field. + +Unexported structs that use a mutex to protect fields of the struct may embed +the mutex. + + + + + + + + + + + +
+ +```go +type smap struct { + sync.Mutex // only for unexported types + + 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] +} +``` + +
Embed for private types or types that need to implement the Mutex interface.For exported types, use a private field.
+ +### Copy Slices and Maps at Boundaries + +Slices and maps contain pointers to the underlying data so be wary of scenarios +when they need to be copied. + +#### 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. + + + + + + + + + + +
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] = ... +``` + +
+ +#### Returning Slices and Maps + +Similarly, be wary of user modifications to maps or slices exposing internal +state. + + + + + +
BadGood
+ +```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 racy. +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() +``` + +
+ +### Defer to Clean Up + +Use defer to clean up resources such as files and locks. + + + + + +
BadGood
+ +```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 +``` + +
+ +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 miniscule 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`. + +### 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. + + + + + +
BadGood
+ +```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) +``` + +
+ +### 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. + + + + + +
BadGood
+ +```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 +``` + +
+ +There are cases where using the zero value makes sense, for example when the +zero value case is the desirable default behavior. + +```go +type LogOutput int + +const ( + LogToStdout LogOutput = iota + LogToFile + LogToRemote +) + +// LogToStdout=0, LogToFile=1, LogToRemote=2 +``` + + + +### Error Types + +There are various options for declaring errors: + +- [`errors.New`] for errors with simple static strings +- [`fmt.Errorf`] for formatted error strings +- Custom types that implement an `Error()` method +- Wrapped errors using [`"pkg/errors".Wrap`] + +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 [section on error wrapping](#error-wrapping). +- Otherwise, [`fmt.Errorf`] is okay. + + [`errors.New`]: https://golang.org/pkg/errors/#New + [`fmt.Errorf`]: https://golang.org/pkg/fmt/#Errorf + [`"pkg/errors".Wrap`]: https://godoc.org/github.com/pkg/errors#Wrap + +If the client needs to detect the error, and you have created a simple error +using [`errors.New`], use a var for the error. + + + + + +
BadGood
+ +```go +// package foo + +func Open() error { + return errors.New("could not open") +} + +// package bar + +func use() { + if err := foo.Open(); err != nil { + if err.Error() == "could not open" { + // handle + } else { + 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 err == foo.ErrCouldNotOpen { + // handle + } else { + panic("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. + + + + + +
BadGood
+ +```go +func open(file string) error { + return fmt.Errorf("file %q not found", file) +} + +func use() { + if err := open(); err != nil { + if strings.Contains(err.Error(), "not found") { + // handle + } else { + panic("unknown error") + } + } +} +``` + + + +```go +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() { + if err := open(); err != nil { + if _, ok := err.(errNotFound); ok { + // handle + } else { + panic("unknown error") + } + } +} +``` + +
+ +Be careful with exporting custom error types directly since they become part of +the public API of the package. It is preferable to expose matcher functions to +check the error instead. + +```go +// package foo + +type errNotFound struct { + file string +} + +func (e errNotFound) Error() string { + return fmt.Sprintf("file %q not found", e.file) +} + +func IsNotFoundError(err error) bool { + _, ok := err.(errNotFound) + return ok +} + +func Open(file string) error { + return errNotFound{file: file} +} + +// package bar + +if err := foo.Open("foo"); err != nil { + if foo.IsNotFoundError(err) { + // handle + } else { + panic("unknown error") + } +} +``` + + + +### 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 [`"pkg/errors".Wrap`] so that the error message provides + more context and [`"pkg/errors".Cause`] can be used to extract the original + error. +- Use [`fmt.Errorf`] if the callers do not need to detect or handle that + specific error case. + +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: + + + + + +
BadGood
+ +```go +s, err := store.New() +if err != nil { + return fmt.Errorf( + "failed to create new store: %s", err) +} +``` + + + +```go +s, err := store.New() +if err != nil { + return fmt.Errorf( + "new store: %s", 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]. + + [`"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 + +### 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. + + [type assertion]: https://golang.org/ref/spec#Type_assertions + + + + + +
BadGood
+ +```go +t := i.(string) +``` + + + +```go +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. + + [cascading failures]: https://en.wikipedia.org/wiki/Cascading_failure + + + + + +
BadGood
+ +```go +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 ") + os.Exit(1) + } + foo(os.Args[1]) +} +``` + + + +```go +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 ") + os.Exit(1) + } + if err := foo(os.Args[1]); err != nil { + panic(err) + } +} +``` + +
+ +Panic/recover is 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. + +```go +var _statusTemplate = template.Must(template.New("name").Parse("_statusHTML")) +``` + +Even in tests, prefer `t.Fatal` or `t.FailNow` over panics to ensure that the +test is marked as failed. + + + + + +
BadGood
+ +```go +// func TestFoo(t *testing.T) + +f, err := ioutil.TempFile("", "test") +if err != nil { + panic("failed to set up test") +} +``` + + + +```go +// func TestFoo(t *testing.T) + +f, err := ioutil.TempFile("", "test") +if err != nil { + t.Fatal("failed to set up test") +} +``` + +
+ + + +### Use go.uber.org/atomic + +Atomic operations with the [sync/atomic] package operate on the raw types +(`int32`, `int64`, etc.) so it is easy to forget to use the atomic operation to +read or modify the variables. + +[go.uber.org/atomic] adds type safety to these operations by hiding the +underlying type. Additionally, it includes a convenient `atomic.Bool` type. + + [go.uber.org/atomic]: https://godoc.org/go.uber.org/atomic + [sync/atomic]: https://golang.org/pkg/sync/atomic/ + + + + + +
BadGood
+ +```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() +} +``` + +
+ +## Performance + +Performance-specific guidelines apply only to the hot path. + +### Prefer strconv over fmt + +When converting primitives to/from strings, `strconv` is faster than +`fmt`. + + + + + + +
BadGood
+ +```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()) +} +``` + +
+ +``` +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. Instead, perform the +conversion once and capture the result. + + + + + + +
BadGood
+ +```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) +} +``` + +
+ +``` +BenchmarkBad-4 50000000 22.2 ns/op +``` + + + +``` +BenchmarkGood-4 500000000 3.25 ns/op +``` + +
+ +## Style + +### Group Similar Declarations + +Go supports grouping similar declarations. + + + + + +
BadGood
+ +```go +import "a" +import "b" +``` + + + +```go +import ( + "a" + "b" +) +``` + +
+ +This also applies to constants, variables, and type declarations. + + + + + +
BadGood
+ +```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 +) +``` + +
+ +Only group related declarations. Do not group declarations that are unrelated. + + + + + +
BadGood
+ +```go +type Operation int + +const ( + Add Operation = iota + 1 + Subtract + Multiply + ENV_VAR = "MY_ENV" +) +``` + + + +```go +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. + + + + + +
BadGood
+ +```go +func f() string { + var red = color.New(0xff0000) + var green = color.New(0x00ff00) + var blue = color.New(0x0000ff) + + ... +} +``` + + + +```go +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. + + + + + +
BadGood
+ +```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" +) +``` + +
+ +### Package Names + +When naming packages, choose a name that is: + +- All lower-case. No capitals or underscores. +- Does not need to be renamed using named imports at most call sites. +- Short and succinct. Remember that the name is identified in full at every call + site. +- 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/ + +### 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`. + + [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 +element of the import path. + +```go +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. + + + + + +
BadGood
+ +```go +import ( + "fmt" + "os" + + + nettrace "golang.net/x/trace" +) +``` + + + +```go +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. + + + + + +
BadGood
+ +```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 {...} +``` + +
+ +### 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. + + + + + +
BadGood
+ +```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() +} +``` + +
+ +### Unnecessary Else + +If a variable is set in both branches of an if, it can be replaced with a +single if. + + + + + +
BadGood
+ +```go +var a int +if b { + a = 100 +} else { + a = 10 +} +``` + + + +```go +a := 10 +if b { + a = 100 +} +``` + +
+ +### 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. + + + + + +
BadGood
+ +```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" } +``` + +
+ +Specify the type if the type of the expression does not match the desired type +exactly. + +```go +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. +``` + +### Prefix Unexported Globals with _ + +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. + + + + + +
BadGood
+ +```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" +) +``` + +
+ +### Embedding in Structs + +Embedded types (such as mutexes) should be at the top of the field list of a +struct, and there must be an empty line separating embedded fields from regular +fields. + + + + + +
BadGood
+ +```go +type Client struct { + version int + http.Client +} +``` + + + +```go +type Client struct { + http.Client + + version int +} +``` + +
+ +### Use Field Names to initialize Structs + +You should almost always specify field names when initializing structs. This is +now enforced by [`go vet`]. + + [`go vet`]: https://golang.org/cmd/vet/ + + + + + +
BadGood
+ +```go +k := User{"John", "Doe", true} +``` + + + +```go +k := User{ + FirstName: "John", + LastName: "Doe", + Admin: true, +} +``` + +
+ +Exception: Field names *may* be omitted in test tables when there are 3 or +fewer fields. + +```go +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. + + + + + +
BadGood
+ +```go +var s = "foo" +``` + + + +```go +s := "foo" +``` + +
+ +However, there are cases where the default value is clearer when the `var` +keyword is use. [Declaring Empty Slices], for example. + + [Declaring Empty Slices]: https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices + + + + + +
BadGood
+ +```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) + } + } +} +``` + +
+ +### 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. + + + + + +
BadGood
+ + ```go + if x == "" { + return []int{} + } + ``` + + + + ```go + if x == "" { + return nil + } + ``` + +
+ +- To check if a slice is empty, always use `len(s) == 0`. Do not check for + `nil`. + + + + + +
BadGood
+ + ```go + func isEmpty(s []string) bool { + return s == nil + } + ``` + + + + ```go + func isEmpty(s []string) bool { + return len(s) == 0 + } + ``` + +
+ +- The zero value (a slice declared with `var`) is usable immediately without + `make()`. + + + + + +
BadGood
+ + ```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) + } + ``` + +
+ +### Reduce Scope of Variables + +Where possible, reduce scope of variables. Do not reduce the scope if it +conflicts with [Reduce Nesting](#reduce-nesting). + + + + + +
BadGood
+ +```go +err := ioutil.WriteFile(name, data, 0644) +if err != nil { + return err +} +``` + + + +```go +if err := ioutil.WriteFile(name, data, 0644); 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. + + + + + +
BadGood
+ +```go +if data, err := ioutil.ReadFile(name); err == nil { + err = cfg.Decode(data) + if err != nil { + return err + } + + fmt.Println(cfg) + return nil +} else { + return err +} +``` + + + +```go +data, err := ioutil.ReadFile(name) +if err != nil { + return err +} + +if err := cfg.Decode(data); err != nil { + return err +} + +fmt.Println(cfg) +return nil +``` + +
+ +### Avoid Naked Parameters + +Naked parameters in function calls can hurt readability. Add C-style comments +(`/* ... */`) for parameter names when their meaning is not obvious. + + + + + +
BadGood
+ +```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 */) +``` + +
+ +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. + +```go +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) +``` + +### Use Raw String Literals to Avoid Escaping + +Go supports [raw string literals](https://golang.org/ref/spec#raw_string_lit), +which can span multiple lines and include quotes. Use these to avoid +hand-escaped strings which are much harder to read. + + + + + +
BadGood
+ +```go +wantError := "unknown name:\"test\"" +``` + + + +```go +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. + + + + + +
BadGood
+ +```go +sval := T{Name: "foo"} + +// inconsistent +sptr := new(T) +sptr.Name = "bar" +``` + + + +```go +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. + + + + + +
BadGood
+ +```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) +``` + +
+ +### 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. + + [Printf family]: https://golang.org/cmd/vet/#hdr-Printf_family + +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. + +```shell +$ go vet -printfuncs=wrapf,statusf +``` + +See also [go vet: Printf family check]. + + [go vet: Printf family check]: https://kuzminva.wordpress.com/2017/11/07/go-vet-printf-family-check/ + +## Patterns + +### Test Tables + +Use table-driven tests with [subtests] to avoid duplicating code when the core +test logic is repetitive. + + [subtests]: https://blog.golang.org/subtests + + + + + +
BadGood
+ +```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) + }) +} +``` + +
+ +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. + +```go +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. + + + + + +
BadGood
+ +```go +// 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 */) +``` + + + +```go +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, + +- [Self-referential functions and the design of options] +- [Functional options for friendly APIs] + + [Self-referential functions and the design of options]: https://commandcenter.blogspot.com/2014/01/self-referential-functions-and-design.html + [Functional options for friendly APIs]: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis + + \ No newline at end of file