blob: 22e23a4b192c1bbb84a2da913646ec4dd63d1929 [file] [log] [blame]
When in Go, do as Gophers do
Go Conference 2014 autumn
30 Nov 2014
Fumitoshi Ukai
Google Software Engineer - Chrome Infra team
* Go Readability Approver
A team to review Go readability.
- help to learn idiomatic Go though code review
- review code of projects that are not main project of the reviewer
- [[][I joined the team about a year ago]] as 20% time, and reviewed ~200 CLs
- For now, I'm reviewing at most 3 CLs per day, 12 CLs per week.
.image readability/project.png
.caption _Gopher_ by [[][Renée French]]
* What is Readability skill?
Literacy of a programming language.
A skill to read or write *idiomatic* code.
Each programming language has its own preferred style.
In C++, each project chooses a preferred style.
- [[][google]] ([[][chromium]]), [[][webkit]] ([[][blink]])
Don't write Go code as you write code in C++/Java/Python.
Write Go code as Gophers write code.
* Go code should ...
- be articulate, concise.
- provide a simple API.
- have precise comments.
- be readable, top-down code.
.caption by some Googler
.image readability/pkg.png
.caption _Gopher_ by [[][Renée French]]
* Good tools
[[][go fmt]] - format Go programs.
[[][go vet]] - report suspicious code
[[][golint]] - report coding style errors.
[[][godoc]] - browse documenation
# Go code is easy to read for tools too.
* Tools are not enough
# Developer doesn't read code the same as tools.
# Programs(compiler, etc) will read code if its syntax is ok.
# Tools are not enough, have false-positive/false-negative.
# Needs human judgments.
Readable code == easy to recognize, less burden for brain.
Both writer and reader should have readability skills.
Go is very simple ([[][lang spec]] is about 50 pages)
.image readability/gophers5th.jpg
.caption _Gopher_ by [[][Renée French]]
* Readability Reviews
- Any mistakes/bugs?
- Layout?
- Simple code flow?
- Simple API?
.image readability/gopher-ok-no.png
.caption _Gopher_ by [[][Renée French]], and [[][tenntenn]]
* mistakes/bugs
* error check
original code
.code readability/err_regexp_bad.go
.code readability/err_regexp_good.go
- Check error with [[][regexp.MustCompile]].
- Must should be used only in [[][initialization]] (package `var` or `init()`).
- [[][Raw string literal]] makes it easy to read regexp.
* error check: original code
.code readability/err_close_write_bad.go
* error check: revised
.code readability/err_close_write_good.go
- Check error of Close for write.
- No need to use defer, when it's simpler.
* in-band error: original code
.code readability/in-band-error-client.go
.code readability/in-band-error.go
* return value and error: revised
.code readability/val-and-error.go
[[][Return error as error, not as some value]]
* error design
If client doesn't need to distinguish errors, e.g. ok with `err` `!=` `nil` check only.
fmt.Errorf("error in %s", val) or errors.New("error msg")
If client wants to distinguish several errors by error code.
var (
ErrInternal = errors.New("foo: inetrnal error")
ErrBadRequest = errors.New("foo: bad request")
If you want to put detailed information in error.
type FooError struct { /* fields of error information */ }
func (e *FooError) Error() string { return /* error message */ }
&FooError{ /* set error data */ }
Don't use `panic`.
But when you do, use it only within the package, and [[][return error with catching it by recover]].
* nil error
.code readability/nil_error.go
[[][FAQ: Why is my nil error value not equal to nil?]]
[[][interface has 2 data]] (type and value). interface value is nil == both are nil.
* embed interface: original code
.code readability/implement-interface-bad.go
- `scan.Writer` is an interface.
- `ColumnWriter` will have methods of the `scan.Writer` interface (i.e. `ColumnWriter` implements the `scan.Writer` interface), but ...
* check interface implementation: revised
.code readability/implement-interface-good.go
- The original author wanted to check `ColumnWriter` [[][implements]] the `scan.Writer` interface.
* embed interface
If a struct doesn't have a method of a interface explicitly, the interface is embedded in the struct, and you didn't set the interface field to a concrete value (i.e. the interface field value is nil), the method call will panic.
.code readability/nil_interface_en.go
It would be useful in a test when you want to implement only a subset of methods in the huge interface.
* Readable layout
* layout of fields in struct: original code
.code readability/struct-field-bad.go
* layout of fields in struct: revised
.code readability/struct-field-good.go
- Organize fields in groups, with blank lines between them.
- Put [[][sync.Mutex]] in top of a block of fields that the mutex protects.
* Long line
.code readability/long-line-fold.go
* Merge into one line
.code readability/long-line-nofold.go
- [[][No rigid line length limit]]
- though, can't we make it shorter?
* Choose concise names
[[][Choose good name in the context]]
- Long names are not always better than short names.
Short and accurate names.
- [[][SamplingServer in sampling package is stutter]]. Name `Server`, which clients will write as `sampling.Server`.
- Use [[][one or two letters for receiver names]].
- Use short names for parameters since type name will give some information.
- Use descriptive names for basic types, though.
- Use short names for local variables: prefer `i` to `index`, `r` to `reader`.
- Short names should be fine if function is not long.
* Revised one line version
.code readability/long-line-short.go
* top-down code
* conditional branch
- [[][Keep the normal code path at a minimal indentation.]]
original code
.code readability/if-else-bad.go
.code readability/if-else-good.go
* conditional branch (2): original code
.code readability/resthandler.go
* conditional branch (2): revised
.code readability/resthandler-fix2.go
- factor out function.
* conditional branch (3): original code
.code readability/if-switch-bad.go
* conditional branch (3): revised
.code readability/if-switch-good.go
- use [[][switch]]
* Simpler code
* time.Duration
Use [[][time.Duration]] ([[][flag.Duration]]) rather than `int` or `float` to represent time duration.
original code
.code readability/time_duration_bad.go
.code readability/time_duration_bad1.go
.code readability/time_duration_bad2.go
.code readability/time_duration_good.go
- Don't write unnecessary type conversion.
- Since [[][const is untyped]], no need to convert 30 to `time.Duration`.
- Don't write unnecessary comments.
* sync.Mutex and sync.Cond: original code
.code readability/close-cond-bad.go
* chan: revised
.code readability/close-cond-good.go
- You could use [[][chan]], instead of [[][sync.Mutex]] and [[][sync.Cond]].
* reflect: original code
.code readability/reflect-bad.go
* without reflect: revised
.code readability/reflect-good.go
- Don't use [[][reflect]], if you know the type at compilation time.
* Test
* Test code
.code readability/test-pattern_en.go
- [[][Have helpful test failure messages]]
- Don't create yet-another assert function. Use existing programming language features.
- Write [[][an example test]] rather than writing how to use API in a doc comment.
.code readability/example_test.go
* Comment
* Comment
[[][Write package comment.]] Write command comment in main package.
[[][Write comments on exported names.]]
[[][Doc comment should be a complete sentence that starts with the name being declared.]]
// Package math provides basic constants and mathematical functions.
package math
// A Request represents a request to run a command.
type Request struct { ..
// Encode writes the JSON encoding of req to w.
func Encode(w io.Writer, req *Request) {
Browse with [[][godoc]]
$ godoc bytes Buffer
$ godoc -http=:6060 # http://localhost:6060/pkg
If you feel comments are unclear or hard to write concisely, reconsider your API design.
* API design
Important to choose a good package name.
- e.g. package `util` is not good name, since most packages are utilities of something.
# split it into smaller packages, or merge it in bigger package, or wrong package boundary.
Make API simple.
- [[][Use multiple returns]]. Don't use pointers as output parameters.
- Don't use pointer to slice, map, chan or interface.
- [[][Return error as error]]: [['t_Panic][don't panic]]
- Implement common interfaces ([[][fmt.Stringer]], [[][io.Reader]] and so on) if they match your code.
- Use interfaces for parameters. They makes it easier to test. e.g.: If a function reads from a file, use [[][io.Reader]] as a parameter instead of [[][*os.File]].
- Prefer synchronous API to async API: refrain from using chan across package boundary.
- Clients can easily run synchronous API concurrently with goroutine and chan.
* To write readable code
* Code is communication
Be articulate:
- Choose good names.
- Design simple APIs.
- Write precise documentation.
- Don't be too complicated.
.image readability/talks.png
.caption _Gopher_ by [[][Renée French]]
* When you write code
Keep in mind
- Articulate, concise.
- Provide simple API.
- Have precise comment.
- Readable, top-down code.
* References
- Effective Go: [[][]]
- standard package: [[][]]
- Code Review Comments: [[][]]
- Go for gophers: [[][]]
- What's in a name? [[][]]
- Organizing Go code: [[][]]
.image readability/ref.png
.caption _Gopher_ by [[][Renée French]]