When in Go, do as Gophers do
Go Conference 2014 autumn
30 Nov 2014

Fumitoshi Ukai
Google Software Engineer - Chrome Infra team
ukai@google.com
https://plus.google.com/+FumitoshiUkai
@fumitoshi_ukai

* 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

- [[http://blogger.ukai.org/2013/12/code-readability.html][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 [[http://www.reneefrench.com][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.

- [[http://google-styleguide.googlecode.com/svn/trunk/cppguide.html][google]] ([[http://www.chromium.org/developers/coding-style][chromium]]), [[https://www.webkit.org/coding/coding-style.html][webkit]] ([[http://www.chromium.org/blink/coding-style][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.

_"_Want_to_understand_something_in_google_servers?_Read_the_Go_implementation!_"_

.caption by some Googler

.image readability/pkg.png
.caption _Gopher_ by [[http://www.reneefrench.com][Renée French]]


* Good tools

[[https://golang.org/cmd/gofmt/][go fmt]] - format Go programs.
[[https://godoc.org/golang.org/x/tools/cmd/vet][go vet]] - report suspicious code
[[https://github.com/golang/lint][golint]] - report coding style errors.
[[http://blog.golang.org/godoc-documenting-go-code][godoc]] - browse documentation

# 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 ([[https://golang.org/ref/spec][lang spec]] is about 50 pages)

.image readability/gophers5th.jpg
.caption _Gopher_ by [[http://www.reneefrench.com][Renée French]]

* Readability Reviews

- Any mistakes/bugs?
- Layout?
- Simple code flow?
- Simple API?

.image readability/gopher-ok-no.png
.caption _Gopher_ by [[http://www.reneefrench.com][Renée French]], and [[https://github.com/tenntenn/gopher-stickers/][tenntenn]]

* mistakes/bugs

* error check

original code

.code readability/err_regexp_bad.go

revised

.code readability/err_regexp_good.go

- Check error with [[https://golang.org/pkg/regexp/#MustCompile][regexp.MustCompile]].
- Must should be used only in [[http://golang.org/ref/spec#Package_initialization][initialization]] (package `var` or `init()`).

- [[http://golang.org/ref/spec#String_literals][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

[[http://golang.org/doc/effective_go.html#multiple-returns][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: internal 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 [[http://golang.org/doc/effective_go.html#recover][return error with catching it by recover]].

* nil error

.code readability/nil_error.go

[[https://golang.org/doc/faq#nil_error][FAQ: Why is my nil error value not equal to nil?]]

[[http://blog.golang.org/laws-of-reflection][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` [[http://golang.org/doc/effective_go.html#blank_implements][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 [[https://golang.org/pkg/sync/#Mutex][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

- [[https://golang.org/s/comments#Line_Length][No rigid line length limit]]
- though, can't we make it shorter?

* Choose concise names

[[https://golang.org/s/comments#Variable_Names][Choose good name in the context]]

- Long names are not always better than short names.

Short and accurate names.

- [[https://golang.org/s/comments#Package_Names][SamplingServer in sampling package is stutter]]. Name `Server`, which clients will write as `sampling.Server`.
- Use [[https://golang.org/s/comments#Receiver_Names][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

- [[https://golang.org/s/comments#Indent_Error_Flow][Keep the normal code path at a minimal indentation.]]

original code

.code readability/if-else-bad.go

revised

.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 [[http://golang.org/ref/spec#Switch_statements][switch]]

* Simpler code

* time.Duration

Use [[https://golang.org/pkg/time/#Duration][time.Duration]] ([[https://golang.org/pkg/flag/#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

revised

.code readability/time_duration_good.go

- Don't write unnecessary type conversion.
- Since [[http://blog.golang.org/constants][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 [[http://golang.org/ref/spec#Channel_types][chan]], instead of [[https://golang.org/pkg/sync/#Mutex][sync.Mutex]] and [[https://golang.org/pkg/sync/#Cond][sync.Cond]].

* reflect: original code

.code readability/reflect-bad.go

* without reflect: revised

.code readability/reflect-good.go

- Don't use [[https://golang.org/pkg/reflect/][reflect]], if you know the type at compilation time.

* Test

* Test code

.code readability/test-pattern_en.go

- [[https://golang.org/s/comments#Useful_Test_Failures][Have helpful test failure messages]]
- Don't create yet-another assert function. Use existing programming language features.
- Write [[https://golang.org/pkg/testing/#hdr-Examples][an example test]] rather than writing how to use API in a doc comment.

.code readability/example_test.go

* Comment

* Comment

[[https://golang.org/s/comments#Package_Comments][Write package comment.]]  Write command comment in main package.
[[https://golang.org/s/comments#Doc_Comments][Write comments on exported names.]]
[[https://golang.org/s/comments#Comment_Sentences][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 [[http://blog.golang.org/godoc-documenting-go-code][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.

- [[http://golang.org/doc/effective_go.html#multiple-returns][Use multiple returns]]. Don't use pointers as output parameters.
- Don't use pointer to slice, map, chan or interface.
- [[http://golang.org/doc/effective_go.html#multiple-returns][Return error as error]]: [[https://golang.org/s/comments#Don't_Panic][don't panic]]
- Implement common interfaces ([[https://golang.org/pkg/fmt/#Stringer][fmt.Stringer]], [[https://golang.org/pkg/io/#Reader][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 [[https://golang.org/pkg/io/#Reader][io.Reader]] as a parameter instead of [[https://golang.org/pkg/os/#File][*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 [[http://www.reneefrench.com][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: [[https://golang.org/doc/effective_go.html][https://golang.org/doc/effective_go.html]]
- standard package:  [[https://golang.org/pkg/][https://golang.org/pkg/]]
- Code Review Comments:  [[https://golang.org/s/comments][https://golang.org/s/comments]]

- Go for gophers: [[http://go.dev/talks/2014/go4gophers.slide][http://go.dev/talks/2014/go4gophers.slide]]
- What's in a name? [[http://go.dev/talks/2014/names.slide][http://go.dev/talks/2014/names.slide]]
- Organizing Go code: [[http://go.dev/talks/2014/organizeio.slide][http://go.dev/talks/2014/organizeio.slide]]

.image readability/ref.png
.caption _Gopher_ by [[http://www.reneefrench.com][Renée French]]