Go Code Review Patterns

This skill provides Go code review patterns synthesized from 4,100+ actual PR review comments spanning 4.5 years. These patterns represent real-world feedback from experienced Go engineers reviewing production code in a large monorepo.

Pattern Frequency Analysis

Based on analysis of 16,427 Go-specific review comments:

• Testing patterns: 3,875 comments (24%)

• Proto/gRPC patterns: 2,417 comments (15%)

• Code suggestions: 1,930 comments (12%)

• Naming patterns: 1,165 comments (7%)

• Error handling: 1,127 comments (7%)

• Code simplification: 1,119 comments (7%)

• Nit comments: 955 comments (6%)

• Documentation: 821 comments (5%)

• Concurrency: 674 comments (4%)

Error Handling

Use Sentinel Errors with errors.Is/As

Create package-level sentinel errors instead of comparing strings:

// Good - sentinel error var errNoKubeClientAvailable = errors.New("no kubernetes client available")

// In test require.ErrorIs(t, err, errNoKubeClientAvailable)

// Bad - string comparison (flaky) if err.Error() == "no kubernetes client available" { ... }

Real review comment: "Comparing strings is flaky. Better use constant errors."

Error Wrapping Style (Uber Guide)

Follow Uber's error wrapping guide:

// Good - lowercase, no "failed to" prefix, add context return fmt.Errorf("get ConfigMap %s/%s: %w", namespace, name, err) return fmt.Errorf("create user %s: %w", userID, err)

// Bad - generic or redundant prefixes return fmt.Errorf("failed to process: %w", err) return fmt.Errorf("error: %w", err) return fmt.Errorf("unable to connect: %w", err)

Real review comment: "nit: avoid 'failed to' https://github.com/uber-go/guide/blob/master/style.md#error-wrapping"

Use errors.New for Static Errors

// Good - static error return nil, errors.New("only UNUSED usage is currently supported")

// Bad - fmt.Errorf without formatting return nil, fmt.Errorf("only UNUSED usage is currently supported")

Use errors.Join for Multiple Errors

// Good - errors.Join for lists var errs []error for _, item := range items { if err := validate(item); err != nil { errs = append(errs, err) } } return errors.Join(errs...)

// Bad - excessive wrapping for _, item := range items { if err := validate(item); err != nil { allErr = fmt.Errorf("%w; %v", allErr, err) } }

Real review comment: "Instead of doing this, declare var errs []error and append normally there. Then use errors.Join when returning the error."

Return Early on Error

// Good - return on error if err != nil { return nil, fmt.Errorf("get config: %w", err) } // continue with happy path

// Bad - continue after potential error if err != nil { // log and continue - don't do this! }

Code Simplification

Reduce Nesting with Early Returns

Apply Uber style guide nesting reduction:

// Good - guard clauses configMap, err := h.GetConfigMap(ctx, namespace, name) if err == nil { return configMap, false, nil }

if !apierrors.IsNotFound(err) { return nil, false, fmt.Errorf("get ConfigMap: %w", err) }

// continue with creation...

// Bad - deeply nested if err != nil { if apierrors.IsNotFound(err) { newConfigMap, createErr := createFn() if createErr == nil { // ... } } }

Real review comment: "nit: avoid indent by reversing logic and early continuation"

Invert Conditions for Early Return

// Good - invert and return early if !isValid(input) { return errors.New("invalid input") } // main logic here

// Bad - unnecessary nesting if isValid(input) { // main logic here } else { return errors.New("invalid input") }

Use continue in Loops

// Good - early continue for _, item := range items { if item == nil { continue } if !item.IsValid() { continue } process(item) }

// Bad - nested for _, item := range items { if item != nil { if item.IsValid() { process(item) } } }

Extract Helper Functions for Long Methods

Real review comment: "nit: could you pull this to a helper method? The level of nesting here makes it hard to follow"

Testing

Use require Over Manual Checks

// Good require.NoError(t, err) require.Equal(t, expected, actual) require.NotEmpty(t, output)

// Bad if err != nil { t.Fatalf("unexpected error: %v", err) }

Real review comment: "consider using require.NoError(t, err) instead, and below"

Use require.ErrorIs for Sentinel Errors

// Good - type-safe error checking require.ErrorIs(t, err, errNotFound)

// Bad - string comparison require.ErrorContains(t, err, "not found")

Use testdata Folders for Expected Values

// Good - use testdata folder func TestTranslate(t *testing.T) { input := mustReadFile(t, "testdata/input.yaml") expected := mustReadFile(t, "testdata/output.yaml") actual := translate(input) require.YAMLEq(t, expected, actual) }

Real review comment: "let's add proper test assertion files in a testdata folder so that we can verify the contents of what's being returned"

Use require.JSONEq or require.YAMLEq

// Good - semantic comparison require.JSONEq(t, expectedJSON, actualJSON) require.YAMLEq(t, expectedYAML, actualYAML)

// Bad - byte-level comparison require.Equal(t, []byte(expected), actual)

Real review comment: "instead of all this, could you just serialise both actual/expected as JSON or YAML and then do require.JSONEq or YAMLEq "

Avoid Testing Only Mock Behavior

// Bad - only tests mock setup mock := &MockStore{returnValue: "expected"} svc := NewService(mock) result := svc.Get() require.Equal(t, "expected", result) // Tests nothing real

Real review comment: "This indirection layer you're adding is kinda pointless... adding a layer there to mock grpc stuff provides no value"

Use embed.FS for Test Data

//go:embed testdata/*.yaml var testdataFS embed.FS

func TestCases(t *testing.T) { entries, _ := testdataFS.ReadDir("testdata") for _, e := range entries { // ... } }

Real review comment: "What about having just one variable with all testdata files and declare it as an embed.FS , then in the table test you just pass the name of the files?"

Concurrency

Use RWMutex for Read-Heavy Workloads

// Good type Cache struct { mu sync.RWMutex items map[string]Item }

func (c *Cache) Get(key string) (Item, bool) { c.mu.RLock() defer c.mu.RUnlock() item, ok := c.items[key] return item, ok }

// Bad - using Mutex for reads func (c *Cache) Get(key string) (Item, bool) { c.mu.Lock() defer c.mu.Unlock() // ... }

Real review comment: "nit: if you change the mu to a RWMutex then you can"

Avoid sync.Once Misuse in Tests

// Bad - new instance per test does nothing func TestFoo(t *testing.T) { var once sync.Once // Different instance each test! once.Do(setup) }

Real review comment: "you're using sync.Once incorrectly, as you're using a different instance in each test, which does nothing. This is completely wrong."

Avoid init() - It's an Anti-Pattern

// Bad - hidden global state mutation func init() { globalConfig = loadConfig() }

// Good - explicit initialization func main() { config := loadConfig() // pass config explicitly }

Real review comment: "init is an anti-pattern in tests"

Use Pointer Receivers with Mutexes

// Good - pointer receiver prevents struct copy func (w *Wrapper) Name() string { w.mu.RLock() defer w.mu.RUnlock() return w.name }

// Bad - value receiver copies mutex (data race!) func (w Wrapper) Name() string { return w.name }

Document Thread Safety Assumptions

// unsafeConfigRepository bypasses permission checks. // Safe because this API only allows org-owners to call it. unsafeConfigRepository persistence.UnsafeConfigRepository

Naming Conventions

Avoid Get Prefix (Effective Go)

Per Effective Go Getters:

// Good func (u *User) Name() string { ... } func (c *Config) Timeout() time.Duration { ... }

// Bad func (u *User) GetName() string { ... } func (c *Config) GetTimeout() time.Duration { ... }

Real review comment: "nit: avoid get prefix https://golang.org/doc/effective_go#Getters"

Capitalize Initialisms

Per Go Code Review Comments:

// Good var userID string var xmlAPI string var httpClient *http.Client

// Bad var userId string var xmlApi string var httpClient *http.Client // Actually correct, HTTP is initialism

Real review comment: "nit: capitalize acronyms https://github.com/golang/go/wiki/CodeReviewComments#initialisms"

Use Short Receiver Names

// Good func (u *User) Name() string { ... } func (s *Server) Handle(ctx context.Context) { ... }

// Bad func (user *User) Name() string { ... } func (server *Server) Handle(ctx context.Context) { ... }

Avoid Underscore Prefixes

Go doesn't use underscore prefixes for scope:

// Good type Config struct { registrationDone bool }

// Bad - not idiomatic Go type Config struct { _registrationDone bool }

Real review comment: "nit: in Go we don't see this kind of underscore prefixes to hint scope like in other langs"

Use Must Prefix for Panicking Functions

// Good - clear panic semantics func MustCreateDir(path string) { if err := os.MkdirAll(path, 0755); err != nil { panic(err) } }

// Bad - unclear panic behavior func CreateDir(path string) { // panics on error - not obvious from name }

Real review comment: "nit: this kind of func that panics or exits usually are named with the Must prefix"

Interface Design

Define Interfaces at Point of Use

Per Google Go Decisions:

// Good - interface at consumer func NewProvider(cache JWKSCache) *Provider { ... }

// The interface is defined where consumed, not where implemented

Real review comment: "nit: wondering if we could unexport this by applying https://google.github.io/styleguide/go/decisions#interfaces"

Accept Interfaces, Return Concrete Types

// Good func NewService(store Store) *Service { ... } func (s *Service) Process() *Result { ... }

// Bad - returning interface func NewService(store Store) ServiceInterface { ... }

Keep Interfaces Small

// Good - focused interface type Store interface { Get(id string) (*User, error) }

// Bad - kitchen sink type Store interface { Get(id string) (*User, error) List() ([]*User, error) Create(*User) error Update(*User) error Delete(id string) error Count() int // ... }

Use Interface Assertions

// Good - compile-time interface check var _ http.Handler = (*MyHandler)(nil)

// In type definition file type MyHandler struct { ... }

Real review comment: "prefer an interface assertion"

Protobuf and gRPC

Always Use Getters for Proto Fields

// Good - nil-safe name := msg.GetName() config := response.GetConfig()

// Check before use if msg.GetConfig() != nil { // use config }

// Bad - can panic on nil name := msg.Name config := response.Config

Real review comment: "Does onboardingCfg come from a proto? Could we use Get methods to avoid nil pointers?"

Handle Nil Proto Messages

// Good - nil-safe chain if op.Spec.GetIstio() == nil || len(op.Spec.GetIstio().GetRevisions()) == 0 { return }

// Better - use Get methods if len(op.Spec.GetIstio().GetRevisions()) == 0 { return }

Real review comment: "if op.Spec.GetIstio() == nil || len(op.Spec.GetIstio().Revisions) == 0 { -> if len(op.Spec.GetIstio().GetRevisions()) == 0 { "

Nil Safety

Check Nil Before Dereferencing

// Good if cp.Spec.TelemetryStore != nil { // use TelemetryStore }

// Bad - potential panic value := cp.Spec.TelemetryStore.RetentionDays

Real review comment: "The function accesses cp.Spec.TelemetryStore without checking if it's nil first. This could cause a nil pointer panic"

Return nil Instead of Empty Slice When Appropriate

// Good - return nil, callers can range and len() safely func (r *Repo) List() ([]Item, error) { if notFound { return nil, nil // nil slice is fine } return items, nil }

// Note: you can range over and len() a nil slice safely var s []int = nil for _, v := range s { } // works len(s) == 0 // true

Real review comment: "There is no need to return an empty list here; just return nil . Take into account that you can range over a nil slice and also call len "

Append Handles Nil Slices

// Good - append works on nil var items []Item items = append(items, newItem)

// Use when size unknown if someCondition { items = append(items, item) }

Slice and Map Patterns

Specify Capacity When Known

Per Uber Guide:

// Good - pre-allocate users := make([]User, 0, len(ids)) for _, id := range ids { users = append(users, User{ID: id}) }

// Bad - grows dynamically var users []User for _, id := range ids { users = append(users, User{ID: id}) }

Real review comment: "fyi I've seen this https://github.com/uber-go/guide/blob/master/style.md#specifying-slice-capacity applied in several places in the codebase"

Use maps.Keys and slices.Collect

// Good - use stdlib helpers (Go 1.21+) keys := slices.Collect(maps.Keys(myMap))

// Bad - manual iteration var keys []string for k := range myMap { keys = append(keys, k) }

Real review comment: "use maps.Keys instead?"

Use strings.Cut for Splitting

// Good prefix, suffix, found := strings.Cut(s, "/")

// Bad parts := strings.SplitN(s, "/", 2) if len(parts) < 2 { ... }

Named Returns

Avoid Named Returns Unless Necessary

Per Go Code Review Comments:

// Good - clear without named returns func process() (Config, error) { cfg := Config{} if err := load(&cfg); err != nil { return Config{}, err } return cfg, nil }

// Named returns only when they improve readability // e.g., multiple returns of same type func coords() (x, y int) { ... }

Real review comment: "I don't think named results help here, as per https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters"

Logging

Use Structured Logging

// Good - structured fields logger.Info("processing request", "user_id", userID, "duration_ms", duration.Milliseconds(), )

// Bad - string formatting log.Printf("processing request user=%s duration=%v", userID, duration)

Don't Log at Inappropriate Levels

// Bad - too verbose for info level logger.Info("polling every 5 seconds") // Runs every 5 seconds!

// Good - use debug for frequent events logger.Debug("polling", "interval", "5s")

Real review comment: "Probably too verbose for info level? They will appear every 5 seconds"

Documentation

Follow Go Doc Conventions

Per Go Blog - Godoc:

// Good - starts with function name // Process validates and transforms the input data. // It returns an error if validation fails. func Process(data []byte) (*Result, error) { ... }

// Bad - doesn't start with name // This function validates and transforms data. func Process(data []byte) (*Result, error) { ... }

Real review comment: "Follow the go comment conventions when adding comments to methods"

Remove Commented Code

// Bad - leaving commented code // oldFunction() // Removed in PR #1234

// Good - just delete it

Real review comment: "Remove commented code"

Common Review Comments

nit: Prefix for Minor Issues

Use nit: prefix for non-blocking suggestions:

• Typos, style preferences

• Minor refactoring opportunities

• Suggestions that are "nice to have"

References to Style Guides

Common references in reviews:

• Uber Go Style Guide

• Effective Go

• Go Code Review Comments

• Google Go Style Guide

Review Checklist

When reviewing Go code, check:

Error Handling

• Errors wrapped with context (no "failed to" prefix)

• Sentinel errors for known conditions

• No swallowed errors

• errors.Join for multiple errors

Code Quality

• Guard clauses / early returns

• No deep nesting (max 2-3 levels)

• Functions under ~40 lines

• Consistent naming (no Get prefix)

Testing

• Uses require library

• Uses require.ErrorIs for sentinel errors

• Test data in testdata/ folder

• No tests of mock behavior only

Thread Safety

• Shared state protected

• RWMutex for read-heavy access

• Pointer receivers with mutexes

• No sync.Once misuse

Proto/gRPC

• Uses Get methods (nil-safe)

• Nil checks before dereferencing

Style

• Capitalizes initialisms (ID, URL, HTTP)

• No underscore prefixes

• Must prefix for panicking functions

• Interfaces at point of use

Source: 4,100+ PR review comments from tetrateio/tetrate (2020-2025)