Idiomatic Go

When reviewing Go code, if I run into a situation where I see an unnecessary deviation from idiomatic Go style or best practice, I add an entry here complete with some rationale, and link to it.

I can do this for the smallest and most subtle of details, since I care about Go a lot. I can reuse this each time the same issue comes up, instead of having to re-write the rationale multiple times, or skip explaining why I make a given suggestion.

You can view this as my supplement to https://github.com/golang/go/wiki/CodeReviewComments.

This page is generated from the entries with label "Accepted" here. If you'd like to add a new suggestion, please provide convincing rationale and references (e.g., links to places in Go project that support your suggestion), and open a new issue. It'll show up here when I add an "Accepted" label.

Use consistent spelling of certain words

Do this:

// marshaling
// unmarshaling
// canceling
// cancelation

Don't do this:

// marshalling
// unmarshalling
// cancelling
// cancellation

For consistency with the Go project. These words have multiple valid spellings. The Go project picks one. See CL 14150, issue 17938. Also compare this vs this, and this vs this.

0 comments

Single spaces between sentences

Do this:

// Sentence one. Sentence two.

Don't do this:

// Sentence one.  Sentence two.

For consistency with Go style. See CL 20022.

0 comments

Error variable naming

Do this:

// Package level exported error.
var ErrSomething = errors.New("something went wrong")

func main() {
	// Normally you call it just "err",
	result, err := doSomething()
	// and use err right away.

	// But if you want to give it a longer name, use "somethingError".
	var specificError error
	result, specificError = doSpecificThing()

	// ... use specificError later.
}

Don't do this:

var ErrorSomething = errors.New("something went wrong")
var SomethingErr = errors.New("something went wrong")
var SomethingError = errors.New("something went wrong")

func main() {
	var specificErr error
	result, specificErr = doSpecificThing()

	var errSpecific error
	result, errSpecific = doSpecificThing()

	var errorSpecific error
	result, errorSpecific = doSpecificThing()
}

For consistency. See https://talks.golang.org/2014/names.slide#15.

0 comments

For brands or words with more than 1 capital letter, lowercase all letters

Do this:

// Exported.
var OAuthEnabled bool
var GitHubToken string

// Unexported.
var oauthEnabled bool
var githubToken string

Don't do this:

// Unexported.
var oAuthEnabled bool
var gitHubToken string

Lowercase all letters, not only the first letter, of the first word when you want to make an unexported version.

For consistency with what the Go project does, and because it looks nicer. oAuth is just weird.

2 comments

Comments for humans always have a single space after the slashes

Do this:

// This is a comment
// for humans.

Don't do this:

//This is a comment
//for humans.

For consistency, and because the no space style is reserved for comment pragmas, for example:

//go:generate go run gen.go

Temporarily commented out code should also have no spaces, but it's not meant to be committed.

See https://golang.org/cmd/compile/#hdr-Compiler_Directives.

0 comments

Use defer if its overhead is relatively negligible, or it buys meaningful readability

Don't use it if the readability stays the same, as is the case in short 1-2 exit funcs.

Using it has a fixed performance overhead, which is noticeable for operations that run on the order of nanoseconds.

See https://lk4d4.darth.io/posts/defer/.

1 comments

Use singular form for collection repo/folder name

Do this:

github.com/golang/example/hello
github.com/golang/example/outyet
golang.org/x/mobile/example/basic
golang.org/x/mobile/example/flappy
golang.org/x/image/...
github.com/shurcooL/tictactoe/player/bad
github.com/shurcooL/tictactoe/player/random

Don't do this:

github.com/golang/examples/hello
github.com/golang/examples/outyet
golang.org/x/mobile/examples/basic
golang.org/x/mobile/examples/flappy
golang.org/x/images/...
github.com/shurcooL/tictactoe/players/bad
github.com/shurcooL/tictactoe/players/random

Notice how "example", "image", "player" folder names simply contain many related packages, yet they use singular form.

Because it's consistent with the style the Go project uses.

2 comments

Avoid unused method receiver names

Do this:

func (foo) method() {
	...
}

Don't do this:

func (f foo) method() {
	...
}

If f is unused. It's more readable because it's clear that fields or methods of foo are not used in method.

8 comments

Empty string check

Do this:

if s == "" {
	...
}

Don't do this:

if len(s) == 0 {
	...
}

If you're checking if s is the empty string. Using len(s) is fine for other uses.

The first form is more readable because it's more obvious s is a string and not a slice.

See Russ Cox's comment in https://groups.google.com/forum/#!topic/golang-nuts/7Ks1iq2s7FA:

if I care about "is it this specific string" I tend to write s == "".

2 comments

Mutex hat

struct {
	...

	rateMu     sync.Mutex
	rateLimits [categories]Rate
	mostRecent rateLimitCategory
}

Here, rateMu is a mutex hat. It sits, like a hat, on top of the variables that it protects.

So, without needing to write the comment, the above is implicitly understood to be equivalent to:

struct {
	...

	// rateMu protects rateLimits and mostRecent.
	rateMu     sync.Mutex
	rateLimits [categories]Rate
	mostRecent rateLimitCategory
}

When adding a new, unrelated field that isn't protected by rateMu, do this:

 struct {
 	...

 	rateMu     sync.Mutex
 	rateLimits [categories]Rate
 	mostRecent rateLimitCategory
+
+	common service
 }

Don't do this:

 struct {
 	...

 	rateMu     sync.Mutex
 	rateLimits [categories]Rate
 	mostRecent rateLimitCategory
+	common     service
 }

See https://talks.golang.org/2014/readability.slide#21.

4 comments

Don't os.Exit(2) inside flag.Usage

Do this:

flag.Usage = func() {
	fmt.Fprintf(os.Stderr, "Usage of %s:\n", os.Args[0])
	flag.PrintDefaults()
}

Don't do this:

flag.Usage = func() {
	fmt.Fprintf(os.Stderr, "Usage of %s:\n", os.Args[0])
	flag.PrintDefaults()
	os.Exit(2)
}

The os.Exit call is unnecessary because flag package is already responsible for doing it. See here and here for examples.

6 comments