go-code-review

name: go-code-review description: Go code review checklist based on official Go style guides. When reviewing Go code for style, idioms, and best practices metadata: languages: go

Go Code Review

Comprehensive code review checklist based on official Go style guides.

Reference URLs

For deeper information, fetch these URLs:

https://go.dev/wiki/CodeReviewComments - Go Code Review Comments
https://go.dev/wiki/TestComments - Go Test Comments
https://google.github.io/styleguide/go/decisions - Google Go Style Guide

Formatting

Gofmt

Code formatted with gofmt or goimports
Tabs for indentation (not spaces)

Run: gofmt -w . or goimports -w .

Line Length

No rigid limit, but avoid uncomfortably long lines
Break lines by semantics, not arbitrary length
Long lines often indicate need for shorter names or refactoring

Naming

Package Names

Short, concise, lowercase
No underscores or mixedCaps
Avoid: util, common, misc, api, types, interfaces
Name doesn't stutter: chubby.File not chubby.ChubbyFile

Variable Names

Short for limited scope: c, i, r
Longer for broader scope
Basic rule: further from declaration = more descriptive

Receiver Names

1-2 letter abbreviation of type: c for Client
Consistent across all methods
Never me, this, self

Initialisms

Consistent case: URL or url, never Url
ServeHTTP not ServeHttp
xmlHTTPRequest or XMLHTTPRequest
appID not appId

Getters/Setters

Owner() not GetOwner()
SetOwner() is fine

Interface Names

Single-method interfaces: -er suffix (Reader, Writer)
Multi-method interfaces: descriptive noun

Comments

Comment Sentences

Full sentences starting with name being described
End with period
Doc comments on all exported names

// 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) { ... }

Package Comments

Adjacent to package clause (no blank line)
Full sentences

// Package math provides basic constants and mathematical functions.
package math

Error Handling

Handle Errors

All errors checked (no _, err := Foo() then ignored)
Errors returned or handled, not swallowed

Error Strings

Lowercase (unless proper noun/acronym)
No ending punctuation
fmt.Errorf("something bad") not "Something bad."

Error Flow

Normal path at minimal indentation
Error handling indented

// GOOD
if err != nil {
    return err
}
// normal code

// BAD
if err != nil {
    // error
} else {
    // normal
}

Don't Panic

Use error returns for normal error handling
Panic only for truly unrecoverable situations
Library code almost never panics

Context

First parameter when used: func F(ctx context.Context, ...)
Not stored in structs
Passed through call chain
Use context.Background() only with good reason

Concurrency

Goroutine Lifetimes

Clear when/whether goroutines exit
No goroutine leaks (blocked on unreachable channels)
Documented exit conditions for non-obvious cases

Synchronous Functions

Prefer sync over async
Let callers add concurrency

Race Detection

Tested with go test -race

Interfaces

Interface Location

Defined at point of use (consumer), not implementation
No premature interfaces

// GOOD - consumer defines interface
package consumer
type Thinger interface { Thing() bool }

// BAD - producer defines interface
package producer
type Thinger interface { Thing() bool }
func NewThinger() Thinger { ... }

Return Types

Return concrete types, not interfaces
Let consumers define interfaces as needed

Data Types

Empty Slices

Prefer var t []string (nil slice)
Use t := []string{} only when needed (JSON encoding)

Copying

Don't copy values with pointer methods
Be careful copying structs with slices (aliasing)

Pass Values

Don't pass pointers just to save bytes
*string and *io.Reader usually wrong
Pointers for large structs or mutation

Crypto Rand

crypto/rand for keys, not math/rand
crypto/rand.Text() for random text

Imports

Import Organization

Standard library first, blank line, then others
Grouped with blank lines

import (
    "fmt"
    "os"

    "github.com/foo/bar"
)

Import Naming

Avoid renaming imports unless collision
Rename most local/project-specific import on collision

Import Blank

import _ "pkg" only in main package or tests

Import Dot

Only for external test packages with circular deps
Never in regular code

Testing

Test Failures

Helpful error messages
Show what was wrong, inputs, actual vs expected
Order: actual != expected, message matches

if got != tt.want {
    t.Errorf("Foo(%q) = %d; want %d", tt.in, got, tt.want)
}

Test Names

TestFunctionName format
Subtests with clear names

Table-Driven Tests

Use for multiple test cases
Clear field names in test struct

Named Results

Avoid when repetitive in godoc
Use when clarifying multiple same-type returns
Naked returns only in very short functions

// Avoid
func (n *Node) Parent1() (node *Node) {}

// Better
func (n *Node) Parent1() *Node {}

// OK when clarifying
func (f *Foo) Location() (lat, long float64, err error)

In-Band Errors

Return additional value for validity (error or bool)
Don't use -1, "", nil as error indicators

// GOOD
func Lookup(key string) (value string, ok bool)

// BAD
func Lookup(key string) string  // returns "" on not found

Switch Statements

Direct Evaluation

Use switch with expressions for cleaner multi-way conditionals
Prefer switch over long if-else chains
Switch evaluates cases top-to-bottom, first match wins
No need for break statements (they're automatic)

// GOOD - switch with direct evaluation
switch status := getStatus(); status {
case StatusPending:
    return "pending"
case StatusActive:
    return "active"
case StatusDone:
    return "done"
default:
    return "unknown"
}

// BAD - long if-else chain
if status := getStatus(); status == StatusPending {
    return "pending"
} else if status == StatusActive {
    return "active"
} else if status == StatusDone {
    return "done"
} else {
    return "unknown"
}

Type Switches

Use type switches for interface type assertions
Use comma-ok idiom for safe type assertion

// Type switch
var i interface{} = "hello"
switch v := i.(type) {
case string:
    fmt.Println("string:", v)
case int:
    fmt.Println("int:", v)
default:
    fmt.Println("unknown type")
}

Fallthrough

Avoid fallthrough unless intentional
Use comma-separated cases when multiple values need same handling

// GOOD - comma-separated cases
switch kind {
case "admin", "owner", "superadmin":
    grantFullAccess()
}

Modern Go Idioms (Go 1.21+)

Standard Library Preferences

Uses slices.Sort instead of sort.Slice
Uses slices.Sort instead of sort.Ints/sort.Float64s/sort.Strings
Uses slices.Contains instead of manual search loops
Uses slices.Equal instead of manual slice comparison
Uses slices.Delete instead of append pattern for slice deletion
Uses maps.Clone instead of manual map copy loops
Uses maps.Equal instead of manual map comparison
Uses maps.Keys/maps.Values (Go 1.23+) for iteration

// GOOD - modern idioms
slices.Sort(items)
if slices.Contains(items, target) { ... }
clone := maps.Clone(original)

// BAD - unnecessary manual implementation
sort.Slice(items, func(i, j int) bool { return items[i] < items[j] })
sort.Ints(items)
for _, item := range items {
    if item == target { found = true; break }
}
clone := make(map[K]V, len(original))
for k, v := range original { clone[k] = v }

Generics

Generic functions used for type-safe utilities
Not over-generalized (use specific types when appropriate)
cmp.Ordered constraint used for comparable types
any constraint only when truly type-agnostic

Time-Dependent Code

Uses dependency injection for time.Now() calls
Clock interface pattern for testable time handling
No direct time.Now() in business logic

// GOOD - testable time handling
type Clock interface { Now() time.Time }
type Service struct { clock Clock }

// BAD - untestable
func (s *Service) IsExpired() bool {
    return time.Now().After(s.expiresAt)
}

Iterators (Go 1.23+)

Custom iterators use iter.Seq or iter.Seq2
Range over functions for custom iteration patterns
Iterators return early when yield returns false

WaitGroup.Go (Go 1.24+)

Uses sync.WaitGroup.Go for fire-and-forget goroutines
wg.Go(fn) is equivalent to go func() { wg.Wait(); fn() }() but simpler
Preferred over go func() { defer wg.Done(); fn() }() for simpler error handling

// GOOD - Go 1.24+ WaitGroup.Go pattern
var wg sync.WaitGroup
for _, task := range tasks {
    wg.Go(func() {
        process(task)  // err handled inside
    })
}
wg.Wait()

// ACCEPTABLE - traditional pattern still works
var wg sync.WaitGroup
for _, task := range tasks {
    wg.Add(1)
    go func(t Task) {
        defer wg.Done()
        process(t)
    }(task)
}
wg.Wait()

Dead Code Detection

Bare Blocks

No unnecessary bare blocks { ... } wrapping code without if/for/switch
Bare blocks often indicate leftover code from refactoring
Check for extra indentation levels that could be removed

// BAD - bare block adds unnecessary indentation
msg := buildFlowMessage()
{
    multiModelSection := processMultiModel()
    // ... 30+ lines
}

// GOOD - flatten the code
msg := buildFlowMessage()
multiModelSection := processMultiModel()
// ... (unindented)

Unused Code

No unused imports (run goimports -w .)
No unused variables (compiler will error)
No unreachable code after return/panic/continue
No functions only called from tests (unless test-only with build tag)

Leftover References

No references to removed files (e.g., response.txt after file-based transport removal)
No calls to removed functions
No references to deprecated fields

Unreachable Code

No code after return, panic, break in loops (unless in defer)
No dead branches in if-else after condition simplification
No commented-out code blocks (delete, don't leave)

// BAD - unreachable code after return
func foo() {
    return
    fmt.Println("this never runs")  // dead code
}

// GOOD - remove unreachable code
func foo() {
    return
}

Variable Scope

No variables that could be moved to tighter scope
No shadowing of outer variables unintentionally
Loop variables don't leak outside loop (use for-range copy or explicit variable)

Quick Checklist

For fast reviews, check these critical items:

gofmt - Code is formatted
Errors handled - No ignored errors
Doc comments - Exports documented
slices.Delete - Use for slice deletion (not append pattern)
Naming - Short, idiomatic names
Tests - Exist and helpful failures
No panics - Error returns used
Context - First param when used
Interfaces - At consumer, not producer
Modern idioms - Uses slices/maps packages where applicable
Switch statements - Use switch for multi-way conditionals
Dead code - No bare blocks, unreachable code, or leftover references

Review Output Format

## Code Review: [scope]

### Critical Issues
[Must fix before merge]

### Important Issues
[Should fix]

### Minor Issues
[Nice to have]

### Strengths
[What's done well]

### Verdict: [PASS/NEEDS WORK]