The "Un-Go" Feeling
You open a pull request. The code compiles. The tests pass. The logic handles the edge cases you thought of. But the reviewer leaves a comment that stings: "This feels un-Go."
You stare at the screen. What does that even mean? Is it the variable names? The way you handled the error? The structure of the goroutines? Go has a strong culture around code quality. The language itself pushes you toward clarity, simplicity, and explicit behavior. A code review in Go isn't just about finding bugs. It's about checking alignment with the community's shared mental model.
The official Go project doesn't publish a static PDF checklist. Instead, the ecosystem relies on a battery of tools that enforce the boring stuff automatically. This shifts the review burden. Reviewers stop nitpicking indentation and brace placement. They start looking at architecture, naming, and design. The checklist is a set of commands you run locally, plus a mental model of what idiomatic Go looks like.
Formatting: The Tool Decides
Go has a single, canonical formatting style. There is no debate about two spaces versus four, or where braces go. The gofmt tool handles this. It doesn't just reflow text. It parses your code into an Abstract Syntax Tree, applies formatting rules to the tree, and regenerates the source code. This means gofmt is robust. It won't break your code. It's deterministic. Every Go developer sees the same output.
Most editors run gofmt on save. You don't need to configure it. You don't need to argue about it. If your code doesn't match the style, the tool fixes it. This eliminates "style wars" in code reviews. Reviewers can ignore formatting diffs and focus on logic.
// Before gofmt: messy indentation, inconsistent braces
func badFormat(x int,
y int) int {
return x+y
}
// After gofmt: clean, predictable, standard
func GoodFormat(x int, y int) int {
return x + y
}
Convention aside: gofmt is mandatory in the Go community. Don't argue about indentation. Let the tool decide. If you see a PR with formatting changes, assume the author ran the formatter. If you see a PR without formatting changes but the code is messy, ask them to run gofmt.
Trust gofmt. Argue logic, not formatting.
Static Analysis: Catching Sense Errors
The compiler catches syntax errors. It ensures types match and variables are defined. But the compiler can't catch everything. It doesn't know if you passed a string to a function expecting an integer count. It doesn't know if you forgot to handle a cancellation signal.
go vet is a static analysis tool built into the Go distribution. It runs after compilation and checks for suspicious constructs. It catches bugs that are syntactically valid but logically wrong.
package main
import "fmt"
func main() {
// Vet catches this: format has %d but arg is string
// The compiler allows this because fmt.Printf takes ...any
fmt.Printf("Value is %d\n", "hello")
}
If you run go vet on this code, it rejects the program with printf format %d has arg "hello" of wrong type string. This is a runtime panic waiting to happen. vet finds it now.
go vet checks for many things:
- Printf mismatches.
- Unreachable code.
- Bad lock usage.
- Context misuse.
- Assignment to loop variables captured by closures (in older Go versions).
Run go vet ./... before every commit. It's fast. It catches the stupid mistakes so you don't have to.
The compiler catches syntax. vet catches sense.
Testing: Table-Driven and Race Detection
Go's testing package is minimal. There are no assertions. No mocking frameworks built-in. No complex test runners. You write functions starting with Test, and go test runs them. This simplicity encourages a specific pattern: table-driven tests.
Table-driven tests put test cases in a slice of structs. You loop over the slice and run each case. This reduces boilerplate. You write the test logic once. You add cases by appending to the slice. It scales.
package calculator
import "testing"
// TestAdd verifies the Add function with multiple cases.
func TestAdd(t *testing.T) {
// Define test cases in a slice.
// Each case has a name, inputs, and expected output.
tests := []struct {
name string
a int
b int
want int
}{
{name: "positive numbers", a: 2, b: 3, want: 5},
{name: "negative numbers", a: -1, b: -1, want: -2},
{name: "zero", a: 0, b: 0, want: 0},
}
// Loop over cases and run subtests.
// t.Run creates a subtest for each case.
// This gives detailed output on which case failed.
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := Add(tt.a, tt.b)
if got != tt.want {
t.Errorf("Add(%d, %d) = %d; want %d", tt.a, tt.b, got, tt.want)
}
})
}
}
Convention aside: Use t.Run for subtests. It makes test output readable. When a test fails, you see the case name. You don't have to guess which input caused the failure.
For concurrent code, use the race detector. Run go test -race ./.... The race detector instruments your code to detect data races at runtime. It slows down execution, but it catches bugs that are hard to reproduce. If your code touches shared state, run the race detector.
Table-driven tests scale. Don't write three functions for one case.
Conventions: Errors, Context, and Naming
Go has strong conventions. They aren't enforced by the compiler, but they are enforced by the community. Following them makes your code readable to other Go developers.
Error Handling
Errors are values. Functions return errors explicitly. You check them immediately.
// FetchData retrieves data from a source.
func FetchData(id string) ([]byte, error) {
data, err := db.Query(id)
// Errors are values. Handle them explicitly.
// Wrapping errors preserves the stack trace.
if err != nil {
return nil, fmt.Errorf("fetch data %s: %w", id, err)
}
return data, nil
}
Convention aside: if err != nil { return err } is verbose by design. The community accepts the boilerplate because it makes the unhappy path visible. Don't hide errors. Don't panic in library code. Return the error and let the caller decide.
Context
context.Context carries deadlines, cancellation signals, and request-scoped values. It always goes as the first parameter. It is conventionally named ctx.
// ProcessRequest handles a request with context awareness.
func ProcessRequest(ctx context.Context, w http.ResponseWriter, r *http.Request) {
// Context flows first. It carries deadlines and cancellation.
// Check for cancellation early.
if ctx.Err() != nil {
http.Error(w, "request cancelled", http.StatusServiceUnavailable)
return
}
// Pass context to downstream calls.
result, err := fetchUpstream(ctx, r.URL.Path)
if err != nil {
http.Error(w, "upstream error", http.StatusInternalServerError)
return
}
w.Write(result)
}
Convention aside: Functions that take a context should respect cancellation and deadlines. If a call takes too long, the context will signal cancellation. Check ctx.Err() periodically. Don't block forever.
Context is plumbing. Run it through every long-lived call site.
Naming
Names matter. Go favors short, clear names. Local variables can be short. i for a loop index. f for a file. err for an error. Exported names should be descriptive. CalculateTotalPrice, not CTP.
Receiver names should be one or two letters matching the type. (b *Buffer) Write(...), not (this *Buffer) Write(...). This keeps the code clean.
Public names start with a capital letter. Private names start lowercase. There are no public or private keywords. Capitalization controls visibility.
Pitfalls and Compiler Errors
Go's compiler is strict. It helps you avoid bugs. But it can be confusing if you don't know the rules.
- Unused imports: The compiler rejects programs with unused imports. You get
imported and not used. Rungo mod tidyto clean up dependencies. - Unused variables: You get
variable declared and not used. Use_to discard values intentionally.result, _ := ...says "I considered the second return value and chose to drop it". Use it sparingly with errors. - Type mismatches: The compiler complains with
cannot use x (untyped int constant) as string value in argumentif you pass the wrong type. Go is strongly typed. No implicit conversions. - Goroutine leaks: The compiler can't catch goroutine leaks. A goroutine leaks when it waits on a channel that never gets closed. Always have a cancellation path. Use context or a done channel.
The worst goroutine bug is the one that never logs.
Decision: When to Use What
Code review is a mix of automated checks and human judgment. Use the right tool for the job.
Use gofmt when you want to eliminate style debates. It runs automatically. It enforces a single standard. It saves cognitive load.
Use go vet when you need to catch logic bugs the compiler misses. It checks for printf mismatches, unreachable code, and context misuse. It's fast and built-in.
Use go test -race when your code touches shared state. The race detector finds data races at runtime. It's essential for concurrent code.
Use go mod tidy when you want to clean up dependencies. It removes unused imports and adds missing ones. It keeps the module graph consistent.
Use manual review when checking architecture and naming. Tools can't judge if a function is too complex. They can't judge if a package boundary makes sense. Humans review design.
Use table-driven tests when you have multiple test cases for one function. It reduces boilerplate. It scales. It makes output readable.
Use context when a function might block or needs cancellation. It carries deadlines and signals. It's the standard way to control long-running operations.
Don't fight the type system. Wrap the value or change the design.