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.

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.

Don't write Go code as you write code in C++/Java/Python.
Write Go code as Gophers write code.

Go code should ...

" 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

Gopher by Renée French, and tenntenn

mistakes/bugs

error check

original code

var whitespaceRegex, _ = regexp.Compile("\\s+")

revised

var whitespaceRegex = regexp.MustCompile(`\s+`)

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
}

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
}

check interface implementation: revised

// ColumnWriter is a writer to write ...
type ColumnWriter struct {
    tmpDir string
    // some other fields
}

var _ scan.Writer = (*ColumnWriter)(nil)

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.

Readable layout

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
}

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
}

Choose concise names

Choose good name in the context

Short and accurate names.

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
}

top-down code

conditional branch

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

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

Simpler code

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

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

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

Test

Test code

    // Typical test code
    if got, want := testTargetFunc(input), expectedValue; !checkTestResult(got, want) {
        t.Errorf("testTargetFunc(%v) = %v; want %v", input, got, want)
    }
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

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.

Make API simple.

To write readable code

Code is communication

Be articulate:

Gopher by Renée French

When you write code

Keep in mind

References

Gopher by Renée French

Thank you

Fumitoshi Ukai

Google Software Engineer - Chrome Infra team

Use the left and right arrow keys or click the left and right edges of the page to navigate between slides.
(Press 'H' or navigate to hide this message.)