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

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.

" Want to understand something in google servers? Read the Go implementation! "

by some Googler

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

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

Readability Reviews

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

Gopher by Renée French, and tenntenn

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.

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
}

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.

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
}

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

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

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.

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.
ColumnWriter will have methods of the scan.Writer interface (i.e. ColumnWriter implements the scan.Writer interface), but ...

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.

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.

layout of fields in struct: original code

type Modifier struct {
    pmod   *profile.Modifier
    cache  map[string]time.Time
    client *client.Client
    mu     sync.RWMutex
}

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.

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
}

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?

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

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
}

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

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

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.

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

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

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.

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
}

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.

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

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.

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
}

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.

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.

Code is communication

Be articulate:

Choose good names.
Design simple APIs.
Write precise documentation.
Don't be too complicated.

Gopher by Renée French

When you write code

Keep in mind

Articulate, concise.
Provide simple API.
Have precise comment.
Readable, top-down code.

Thank you

Fumitoshi Ukai

Google Software Engineer - Chrome Infra team

ukai@google.com

https://plus.google.com/+FumitoshiUkai

@fumitoshi_ukai

When in Go, do as Gophers do

Go Conference 2014 autumn

30 November 2014

Go Readability Approver

What is Readability skill?

Go code should ...

Good tools

Tools are not enough

Readability Reviews

mistakes/bugs

error check

error check: original code

error check: revised

in-band error: original code

return value and error: revised

error design

nil error

embed interface: original code

check interface implementation: revised

embed interface

Readable layout

layout of fields in struct: original code

layout of fields in struct: revised

Long line

Merge into one line

Choose concise names

Revised one line version

top-down code

conditional branch

conditional branch (2): original code

conditional branch (2): revised

conditional branch (3): original code

conditional branch (3): revised

Simpler code

time.Duration

sync.Mutex and sync.Cond: original code

chan: revised

reflect: original code

without reflect: revised

Test

Test code

Comment

Comment

API design

To write readable code

Code is communication

When you write code

References

Thank you