When in Go, do as Gophers do Go Conference 2014 autumn 30 November 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. Gopher by Renée French 2

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. 3

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! " by some Googler Gopher by Renée French 4

Good tools go fmt - format Go programs.

go vet - report suspicious code

golint - report coding style errors.

godoc - browse documenation 5

Tools are not enough 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) Gopher by Renée French 6

Readability Reviews Any mistakes/bugs?

Layout?

Simple code flow?

Simple API? Gopher by Renée French, and tenntenn 7

mistakes/bugs 8

error check original code var whitespaceRegex, _ = regexp.Compile("\\s+") revised var whitespaceRegex = regexp.MustCompile(`\s+`) 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. 9

error check: original code func run() error { in, err := os.Open(*input) if err != nil { return err } defer in.Close() out, err := os.Create(*output) if err != nil { return err } defer out.Close() // some code } 10

error check: revised func run() (err error) { in, err := os.Open(*input) if err != nil { return err } defer in.Close() out, err := os.Create(*output) if err != nil { return err } defer func() { if cerr := out.Close(); err == nil { err = cerr } }() // some code } Check error of Close for write.

No need to use defer, when it's simpler. 11

in-band error: original code func proc(it Iterator) (ret time.Duration) { d := it.DurationAt() if d == duration.Unterminated { ret = -1 } else { ret = d } // some code } // duration.Unterminated = -1 * time.Second func (it Iterator) DurationAt() time.Duration { // some code switch durationUsec := m.GetDurationUsec(); durationUsec { case -1: return duration.Unterminated case -2: return -2 default: return time.Duration(durationUsec) * time.Microsecond } return -3 } 12

return value and error: revised var ( ErrDurationUnterminated = errors.new("duration: unterminated") ErrNoDuration = errors.New("duration: not found") ErrNoIteration = errors.New("duration: not interation") ) func (it Iterator) DurationAt() (time.Duration, error) { // some code switch durationUsec := m.GetDurationUsec(); durationUsec { case -1: return 0, ErrDurationUnterminated case -2: return 0, ErrNoDuration default: return time.Duation(durationUsec) * time.Microsecond, nil } return 0, ErrNoIteration } Return error as error, not as some value 13

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. 14

nil error import "log" type FooError struct{} func (e *FooError) Error() string { return "foo error" } func foo() error { var ferr *FooError // ferr == nil return ferr } func main() { err := foo() if err != nil { log.Fatal(err) } } 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. 15

embed interface: original code // Column writer implements the scan.Writer interface. type ColumnWriter struct { scan.Writer tmpDir string // some other fields } scan.Writer is an interface.

is an interface. ColumnWriter will have methods of the scan.Writer interface (i.e. ColumnWriter implements the scan.Writer interface), but ... 16

check interface implementation: revised // ColumnWriter is a writer to write ... type ColumnWriter struct { tmpDir string // some other fields } var _ scan.Writer = (*ColumnWriter)(nil) The original author wanted to check ColumnWriter implements the scan.Writer interface. 17

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. import "fmt" type I interface { Key() string Value() string } type S struct{ I } // S has method sets of I. func (s S) Key() string { return "type S" } func main() { var s S fmt.Println("key", s.Key()) fmt.Println(s.Value()) // runtime error: invalid memory address or nil pointer deference } It would be useful in a test when you want to implement only a subset of methods in the huge interface. 18

Readable layout 19

layout of fields in struct: original code type Modifier struct { pmod *profile.Modifier cache map[string]time.Time client *client.Client mu sync.RWMutex } 20

layout of fields in struct: revised type Modifier struct { client *client.Client mu sync.RWMutex pmod *profile.Modifier cache map[string]time.Time } Organize fields in groups, with blank lines between them.

Put sync.Mutex in top of a block of fields that the mutex protects. 21

Long line package sampling import ( servicepb "foo/bar/service_proto" ) type SamplingServer struct { // some fields } func (server *SamplingServer) SampleMetrics( sampleRequest *servicepb.Request, sampleResponse *servicepb.Response, latency time.Duration) { // some code } 22

Merge into one line package sampling import ( servicepb "foo/bar/service_proto" ) type SamplingServer struct { // some fields } func (server *SamplingServer) SampleMetrics(sampleRequest *servicepb.Request, sampleResponse *servicepb.Response, latency time.Duration) { // some code } No rigid line length limit

though, can't we make it shorter? 23

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 .

, which clients will write as . 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 .

to , to . Short names should be fine if function is not long. 24

Revised one line version package sampling import ( spb "foo/bar/service_proto" ) type Server struct { // some fields } func (s *Server) SampleMetrics(req *spb.Request, resp *spb.Response, latency time.Duration) { // some code } 25

top-down code 26

conditional branch Keep the normal code path at a minimal indentation. original code if _, ok := f.dirs[dir]; !ok { f.dirs[dir] = new(feedDir) } else { f.addErr(fmt.Errorf("...")) return } // some code revised if _, found := f.dirs[dir]; found { f.addErr(fmt.Errorf("...")) return } f.dirs[dir] = new(feedDir) // some code 27

conditional branch (2): original code func (h *RESTHandler) finishReq(op *Operation, req *http.Request, w http.ResponseWriter) { result, complete := op.StatusOrResult() obj := result.Object if complete { status := http.StatusOK if result.Created { status = http.StatusCreated } switch stat := obj.(type) { case *api.Status: if stat.Code != 0 { status = stat.Code } } writeJSON(status, h.codec, obj, w) } else { writeJSON(http.StatusAccepted, h.codec, obj, w) } } 28

conditional branch (2): revised func finishStatus(r Result, complete bool) int { if !complete { return http.StatusAccepted } if stat, ok := r.Object.(*api.Status); ok && stat.Code != 0 { return stat.Code } if r.Created { return http.StatusCreated } return http.StatusOK } func (h *RESTHandler) finishReq(op *Operation, w http.ResponseWriter, req *http.Request) { result, complete := op.StatusOrResult() status := finishStatus(result, complete) writeJSON(status, h.codec, result.Object, w) } factor out function. 29

conditional branch (3): original code func BrowserHeightBucket(s *session.Event) string { browserSize := sizeFromSession(s) if h := browserSize.GetHeight(); h > 0 { browserHeight := int(h) if browserHeight <= 480 { return "small" } else if browserHeight <= 640 { return "medium" } else { return "large" } } else { return "null" } } 30

conditional branch (3): revised func BrowserHeightBucket(s *session.Event) string { size := sizeFromSession(s) h := size.GetHeight() switch { case h <= 0: return "null" case h <= 480: return "small" case h <= 640: return "medium" default: return "large" } } use switch 31

Simpler code 32

time.Duration Use time.Duration (flag.Duration) rather than int or float to represent time duration. original code var rpcTimeoutSecs = 30 // Thirty seconds var rpcTimeout = time.Duration(30 * time.Second) // Thirty seconds var rpcTimeout = time.Duration(30) * time.Second // Thirty seconds revised var rpcTimeout = 30 * time.Second Don't write unnecessary type conversion.

Since const is untyped, no need to convert 30 to time.Duration .

. Don't write unnecessary comments. 33

sync.Mutex and sync.Cond: original code type Stream struct { // some fields isConnClosed bool connClosedCond *sync.Cond connClosedLocker sync.Mutex } func (s *Stream) Wait() error { s.connClosedCond.L.Lock() for !s.isConnClosed { s.connClosedCond.Wait() } s.connClosedCond.L.Unlock() // some code } func (s *Stream) Close() { // some code s.connClosedCond.L.Lock() s.isConnClosed = true s.connClosedCond.L.Unlock() s.connClosedCond.Broadcast() } func (s *Stream) IsClosed() bool { return s.isConnClosed } 34

chan: revised type Stream struct { // some fields cc chan struct{} } func (s *Stream) Wait() error { <-s.cc // some code } func (s *Stream) Close() { // some code close(s.cc) } func (s *Stream) IsClosed() bool { select { case <-s.cc: return true default: return false } } You could use chan, instead of sync.Mutex and sync.Cond. 35

reflect: original code type Layers struct { UI, Launch /* more fields */ string } layers := NewLayers(s.Entries) v := reflect.ValueOf(*layers) r := v.Type() // type Layers for i := 0; i < r.NumField(); i++ { if e := v.Field(i).String(); e != "-" { eid := &pb.ExperimentId{ Layer: proto.String(r.Field(i).Name()), ExperimentId: &e, } experimentIDs = append(experimentIDs, eid) } } 36

without reflect: revised type LayerExperiment struct{ Layer, Experiment string } func (t *Layers) Slice() []LayerExperiment { return []LayerExperiment{ {"UI", t.UI}, {"Launch", t.Launch}, /* more fields */ } } layers := NewLayers(s.Entries).Slice() for _, l := range layers { if l.Experiment != "-" { eid := &pb.ExperimentId{ Layer: proto.String(l.Layer), ExperimentId: proto.String(l.Experiment), } experimentIDs = append(experimentIDs, eid) } } Don't use reflect, if you know the type at compilation time. 37

Test 38

Test code // Typical test code if got, want := testTargetFunc(input), expectedValue; !checkTestResult(got, want) { t.Errorf("testTargetFunc(%v) = %v; want %v", input, got, want) } 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. func ExampleWrite() { var buf bytes.Buffer var pi float64 = math.Pi err := binary.Write(&buf, binary.LittleEndian, pi) if err != nil { fmt.Println("binary.Write failed:", err) } fmt.Printf("% x", buf.Bytes()) // Output: 18 2d 44 54 fb 21 09 40 } 39

Comment 40

API design Important to choose a good package name. e.g. package util is not good name, since most packages are utilities of something. 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: 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. 42

To write readable code 43

Code is communication Be articulate: Choose good names.

Design simple APIs.

Write precise documentation.

Don't be too complicated. Gopher by Renée French 44

When you write code Keep in mind Articulate, concise.

Provide simple API.

Have precise comment.

Readable, top-down code. 45

References Effective Go: https://golang.org/doc/effective_go.html

standard package: https://golang.org/pkg/

Code Review Comments: https://golang.org/s/comments Go for gophers: http://talks.golang.org/2014/go4gophers.slide

What's in a name? http://talks.golang.org/2014/names.slide

Organizing Go code: http://talks.golang.org/2014/organizeio.slide Gopher by Renée French 46