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

Modern Go Idioms (Go 1.21+)

Standard Library Preferences

• Uses slices.Sort instead of sort.Slice
• Uses slices.Contains instead of manual search loops
• Uses slices.Equal instead of manual slice comparison
• 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] })
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

References:

Quick Checklist

For fast reviews, check these critical items:

•gofmt - Code is formatted
•Errors handled - No ignored errors
•Doc comments - Exports documented
•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

Review Output Format

markdown

## 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]