Go Code Review

An informal Go programming language code review guidelines index.

General References

Official

• Style Guide
• Code Review Comments
• Effective Go
• Go Proverbs: Web - Video

3rd Party

• Go Best Practices, Go for Industrial Programming - Video by Peter Bourgon
• Ultimate Go Design Guidelines by Ardan Labs
• Idiomatic Go by Sourcegraph
• The Zen of Go by Dave Cheney
• Practical Go: Real World Advice for Writing Maintainable Go Programs by Dave Cheney
• Uber's Go Style Guide
• Awesome Go Style

About Code Reviewing, Git & GitHub

• The Guidelines for Faster PR Reviews from the Kubernetes project are a must. A quick summary:
  • Do small commits and, even better, small PRs.
  • Use separate PRs for fixes not related to your feature.
  • Add a different commit for review changes so it's easy to review them instead of the whole patch.
  • Test and document your code.
  • Don't add features you don't need.
• Other guidelines:
  • How to Write a Git Commit Message
  • Use a Global .gitignore to avoid coupling personal environment's files. Use a .gitignore Generator when possible.
  • Prefer documentation as code (example test files) over READMEs.
  • Choose a good GitHub merge strategy:
    • Choose merge when:
      • Multiple commits.
      • Deps in different commit.
    • Choose squash if you just have a single commit to avoid the extra merge commit.
  • Have a proper Continuous Integration (CI) to ensure code quality:
    • Tests are in green (don't forget the -race flag).
    • New code or changes in functionality have the corresponding tests (e.g., gocovmerge + codecov.io or coveralls.io).

Linting

Just use golangci-lint. The defaults are fine. The more linters you enable, the more you can avoid nitpicks in PR reviews.

As a formatter, I personally prefer gofumpt: a stricter gofmt.

Design

In companies I worked with that had rich domains, such as video games and logistics, I've seen really good results applying techniques such as Test-Driven Development, Domain-Driven Design, and Command-Query Separation. You can read about it in this presentation.

Other stuff:

• Middleware Pattern:
  • See this go-kit presentation example by Peter Bourgon.
  • And Embrace the Interface video by Tomas Senart (@tsenart).
  • Another good blog post with examples.
• Example: Coupling with Logger, Metrics, etc:
  • Don't log everything, maybe using metrics is better ([blog post by @copyconstruct](

https://medium.com/@copyconstruct/logs-and-metrics-6d34d3026e38)).

• The RED Method for choosing metrics.
• Code is Communication.
• API Design
  • Make Your APIs Sync.
• Always Write the Simplest Code You Can.
• Structs with just one field: has-a -> is-a.
• Avoid Primitive Obsession - C#.
• Accept Interfaces, Return Concrete Types.
• DDD in Go.
• Think About Using Functional Options in Your Constructors.

Package Design

• Learn and follow the stdlib design; read the official doc about Organizing a Go Module.
• Think twice whether your exported types really need to be exported.
• Simplify by reducing as much the number of package dependencies: sometimes duplication is better than the wrong abstraction.

Other references:

• Ben Johnson's article on Standard Package Layout.
• Bill Kennedy's resources:
  • Blog Article.
  • GopherCon India '17 Video.
• Style Guides for Go Pkgs - @rakyll:
  • Use multiple files.
  • Keep types closer to where they are used.
  • Organize by responsibility (e.g., avoid package model).

Error Design

• Design for Errors - Railway Oriented Programming.
  • https://dave.cheney.net/2015/01/26/errors-and-exceptions-redux.
• Indent Error Flow: Early Returns.
  • Talk: Keep the normal code path at a minimal indentation.
• Errors shouldn't be capitalized.
• Prefer testing the public API of your package with the test code in a different package (package whatever_test).
• Modeling:
  • For composition, don't use 3rd party libs since Go 1.13 has wrapping built-in.
    • For multierror support, use Hashi's lib or build it yourself. A stdlib implementation has been implemented but not published yet (as of 1.19.1).
  • Prefer modeling your own errors as values or types; avoid coupling tests to error messages, use errors.Is/As instead.
  • Prefer wrapping for returning errors you don't own (check wrapcheck linter).
• Error Handling Guidelines by Bill K.
  • Handling an error means:
    • The error has been logged.
    • The application is back to 100% integrity.
    • The current error is not reported any longer.
• An http.Handler shouldn't repeat sending an errored response more than once; separate the domain logic from the HTTP layer.
• Don't panic, return errors - especially in libs. For tests or main functions, you can panic, but use function names with Must prefix.

Concurrency

• Coverage: at least happy path. Think about important error paths too.
• Read Golang Concurrency Tricks.
• Go makes concurrency easy enough to be dangerous source.
• Shared mutable state is the root of all evil source.
• How to protect shared mutable data:
  • With Mutex/RWMutex:
    • Theory in Spanish: El Problema de los Lectores/Escritores.
    • mu sync.Mutex over the fields it protects.
    • Use RWMutex if you have (significantly?) more reads than writes.
  • Or avoid sharing state and use message passing via channels:
    • Channel Design.
      • Suspect of buffered channels.
    • Principles of Designing APIs with Channels.
  • Or use the atomic pkg if your state is a primitive (float64 for e.g.):
    • That's a nice thing for testing using spies in concurrent tests for e.g.
  • Or use things like sync.Once or sync.Map (>= Go 1.9).
• Testing:
  • Good concurrent tests are mandatory and run with the -race flag.
  • Use parallel subtests when possible to make tests run faster (official blog post).
    • Must capture range vars! tc := tc // capture range variable (also see next point).
• Watch out when:
  • Launching goroutines inside loops, see Using Goroutines on Loop Iteration Variables.
  • Passing pointers through channels.
  • You see the go keyword around without much explanation and tests:
    • Why was it done?
    • Is there a benchmark for that?
  • Async fire-and-forget logic: does that function return or not a value and/or error that must be handled?
• Goroutine Lifetime:
  • Watch out for leaks: lifetime must be always clear, e.g., terminate a for-select subscribing to context.Done() to finish return.
  • More from official core review.
  • More from Dave Cheney.
• If you write libs, leave concurrency to the caller when possible. See Zen Of Go.
  • Your code will be simpler, and the clients will choose the kind of concurrency they want.
• Don't Expose Channels.
• The Channel Closing Principle: don't close a channel from the receiver side and don't close a channel if the channel has multiple concurrent senders.
• Refs:
  • Pipelines and Cancellation.
  • Advanced Concurrency Patterns.
• Performance:
  • Mutex contention profiling tips from Jaana B Dogan.

HTTP

• Don't use http.ListenAndServe. Use http.Server{} with good defaults as explained in How to Handle HTTP Timeouts in Go.
• An HTTP client must close the response body when finished reading with it.
  • Closing the body must be done after error checking.
• When creating HTTP-based libs, allow injecting an http.Client.

Naming

Be careful when using short names. Usually, there is no problem with the receivers' names or temporary variables like indexes in loops, etc. But if your funcs violate the SRP or you have many args, then you can end up with many short names and it can do more harm than good. If understanding a var name is not straightforward, use longer names.

Other refs:

• Avoid GetSomething Getter Name.
• Named Result Parameters.
• Initialisms - ID, HTTP, etc.
• Receiver Name.

  Don't name result parameters just to avoid declaring a var inside the function.

Testing

Testing on the Toilet short articles by Google have tons of wisdom.

Go Testing By Example presentation from Russ Cox (Go technical lead) is also great. Its best tips in my opinion are:

• Make it easy to add new test cases, by using table-driven tests.
• If you didn't add a test, you didn't fix the bug.
• Code quality is limited by test quality.

Other guidelines:

• Use coverage to find gaps or edge cases.
• Any change in behavior ideally should have a test that covers it; if you do TDD, the test implementation comes first.
• Scope: if tests are complicated think about
  • Refactoring the code.
  • Refactoring the tests.
  • Adding arrange / act / assert comments, if needed.
• Types of tests:
  • Unit.
  • Internal.
  • Integration.
  • Example files.
  • Benchmarks.
    • The Right Way by Daniel Martí.
• Subtests: see concurrency.
• Tags: if you have tagged some tests (e.g., // +build integration) then you need to run your tests with -tags.
• Use t.Run and t.Parallel.
• De-flaking unit tests, by the Kubernetes project.

Test doubles naming (from The Little Mocker by Uncle Bob):

• Dummy: objects are passed around but never actually used.
• Fake: objects actually have working implementations, but usually take some shortcut that makes them not suitable for production (an InMemoryTestDatabase is a good example).
• Stubs: provide canned answers to calls made during the test, usually not responding at all to anything outside what's programmed in for the test.
• Spies: stubs that also record some information based on how they were called. One form of this might be an email service that records how many messages it was sent.
• Mocks: pre-programmed with expectations that form a specification of the calls they are expected to receive. They can throw an exception if they receive a call they don't expect and are checked during verification to ensure they got all the calls they were expecting.

Performance

• Receiver Type: Value or Pointer?.
• Writing High-Performance Go Talk.
• Escape Analysis and Memory Profiling - Bill Kennedy @ GopherCon SG 2017.
• From So You Wanna Go Fast? talk by Tyler Treat:
  • The stdlib provides general solutions, and you should generally use them.
  • Small, idiomatic changes can have a profound performance impact.
  • Learn and use tools from the Go toolchain to analyze the performance.
  • The performance can change a lot between Go versions, review your optimizations.
  • Code is marginal, architecture is material: the big wins come from architecture, do it right first.
  • Mechanical sympathy: know how your abstractions work in your hardware; Go makes this possible.
  • Optimize for the right trade-off: optimizing for performance means trading something else.

Gotchas

• 50 Shades of Go.
• A time.Ticker must be stopped, otherwise, a goroutine is leaked.
• Slices hold a reference to the underlying array as explained here and here. Slices are passed by reference; maybe you think you are returning small data but its underlying array can be huge.
• Interfaces and nil gotcha, see the Understanding Nil talk by Francesc Campoy.

Nitpicks

• How to add EOL at EOF. See why.
  • VSCode.
  • JetBrains' IDEs such as GoLand:
    • Preferences > Editor > General > Ensure every saved file ends with a line break.
  • Sublime Text.
  • Vim: by default, it's on. Read this to disable.