Go Code Review Best Practices

Purpose

This document provides guidelines for reviewing Go code to ensure quality, maintainability, security, and adherence to Go idioms and project conventions.

General Principles

1. Understand Before Reviewing

• Read the entire PR description and linked issue/ticket
• Understand the "why" behind the change, not just the "what"
• Check if the approach aligns with the project's architecture

2. Be Constructive

• Suggest alternatives with explanations
• Ask questions rather than making demands
• Acknowledge good patterns and improvements

3. Focus on Impact

• Prioritize correctness, security, and performance issues
• Don't nitpick minor style issues if automated tools handle them
• Consider the maintainability and testability impact

---

Code Quality Checklist

Correctness

• Logic errors: Does the code do what it's supposed to do?
• Edge cases: Are nil checks, empty slices, zero values handled?
• Boundary conditions: Off-by-one errors, empty input, max values?
• Race conditions: Is concurrent access properly synchronized?
• Resource leaks: Are files, connections, goroutines properly closed?

Error Handling

• All errors checked: No ignored errors without explicit _ =
• Proper error wrapping: Use fmt.Errorf("context: %w", err) for context
• Sentinel errors: Use errors.Is() and errors.As() instead of ==
• Error messages: Lowercase, no punctuation, actionable context
• Panic usage: Only for programmer errors, not for expected failures
• Defer cleanup: Resource cleanup in defer, check errors from Close()

// Good
if err := validateInput(data); err != nil {
    return fmt.Errorf("failed to validate input: %w", err)
}

// Bad - ignored error
_ = db.Close()  // If this is intentional, explain why in a comment

// Good - explicit about why we ignore
_ = resp.Body.Close() // Error already logged by HTTP client

Concurrency Safety

• Shared state: Is mutable shared state protected by locks?
• Goroutine leaks: Do all goroutines have clear termination?
• Context usage: Are contexts passed correctly through call chains?
• Channel usage: Proper sender/receiver patterns, avoid deadlocks
• sync primitives: Correct use of Mutex, RWMutex, WaitGroup, Once
• Data races: Run with -race flag in tests

// Bad - unprotected concurrent map access
var cache = make(map[string]interface{})

func Get(key string) interface{} {
    return cache[key] // RACE!
}

// Good - protected with sync.RWMutex
type Cache struct {
    mu    sync.RWMutex
    items map[string]interface{}
}

func (c *Cache) Get(key string) interface{} {
    c.mu.RLock()
    defer c.mu.RUnlock()
    return c.items[key]
}

Resource Management

• Database connections: Use connection pools, set timeouts
• HTTP clients: Reuse clients, set timeouts, close response bodies
• File handles: Always defer file.Close(), check close errors
• Context cancellation: Respect context deadlines and cancellation
• Goroutine cleanup: Use context or channels to signal termination

// Good - proper resource cleanup
func ProcessFile(ctx context.Context, path string) error {
    f, err := os.Open(path)
    if err != nil {
        return fmt.Errorf("open file: %w", err)
    }
    defer func() {
        if cerr := f.Close(); cerr != nil {
            log.Printf("failed to close %s: %v", path, cerr)
        }
    }()

    // Process file with context
    return process(ctx, f)
}

Performance

• Unnecessary allocations: Avoid allocating in tight loops
• String concatenation: Use strings.Builder for multiple appends
• Slice preallocation: Use make([]T, 0, capacity) when size is known
• Map preallocation: Use make(map[K]V, capacity) when size is known
• N+1 queries: Batch database queries when possible
• Defer in loops: Avoid defer in tight loops (use separate function)

// Bad - allocations in loop
var result string
for _, s := range items {
    result += s // Creates new string each iteration
}

// Good - use strings.Builder
var builder strings.Builder
builder.Grow(totalSize) // Preallocate if size known
for _, s := range items {
    builder.WriteString(s)
}
result := builder.String()

// Bad - defer in loop
for _, file := range files {
    f, _ := os.Open(file)
    defer f.Close() // Defers all accumulate until function exit
    process(f)
}

// Good - separate function
for _, file := range files {
    if err := processFile(file); err != nil {
        return err
    }
}

func processFile(path string) error {
    f, err := os.Open(path)
    if err != nil {
        return err
    }
    defer f.Close() // Runs at end of processFile, not outer loop
    return process(f)
}

Security

• SQL injection: Use parameterized queries, never string concatenation
• Command injection: Validate/sanitize shell command arguments
• Path traversal: Validate file paths, use filepath.Clean()
• Secrets logging: Redact passwords, tokens from logs
• Input validation: Validate all external input (HTTP, env vars, files)
• Cryptography: Use standard library crypto, don't roll your own
• Integer overflow: Check for overflow in calculations (especially 32-bit)

// Bad - SQL injection vulnerability
query := fmt.Sprintf("SELECT * FROM users WHERE name = '%s'", userName)

// Good - parameterized query
query := "SELECT * FROM users WHERE name = ?"
rows, err := db.Query(query, userName)

// Bad - secrets in logs
log.Printf("Connecting with password: %s", password)

// Good - redacted logs
log.Printf("Connecting to database: %s", redactPassword(connString))

---

Go Idioms and Style

Naming Conventions

• Package names: Short, lowercase, single word, no underscores
• Exported identifiers: CamelCase starting with uppercase
• Unexported identifiers: camelCase starting with lowercase
• Acronyms: ID not Id, HTTP not Http, URL not Url
• Getter methods: Value() not GetValue()
• Interfaces: Single-method interfaces often end in -er (Reader, Writer)
• Avoid stuttering: user.UserID → user.ID

// Good
type HTTPServer struct {
    URL      string
    timeout  time.Duration
}

func (s *HTTPServer) Start() error { ... }
func (s *HTTPServer) URL() string { return s.url } // Getter

// Bad
type HttpServer struct {
    Url         string // Should be URL
    TimeOut     time.Duration // Should be timeout
}

func (s *HttpServer) GetUrl() string { ... } // No "Get" prefix

Code Organization

• Package structure: Organize by domain, not by type (avoid models/, utils/)
• File size: Split large files into logical units
• Function length: Keep functions focused and short (< 50 lines guideline)
• Cyclomatic complexity: Reduce nested conditionals
• Interface placement: Define interfaces where they're used, not implemented
• Avoid circular deps: Refactor if packages import each other

// Good - organize by domain
project/
  user/
    user.go
    repository.go
    service.go
  order/
    order.go
    repository.go
    service.go

// Bad - organize by type
project/
  models/
    user.go
    order.go
  repositories/
    user_repository.go
    order_repository.go

Function Design

• Early returns: Use guard clauses to reduce nesting
• Named return values: Use sparingly, mainly for documentation
• Variadic parameters: Last parameter only, consider options pattern
• Context first: Context should be the first parameter
• Options pattern: For functions with many optional parameters

// Good - early returns, less nesting
func Process(ctx context.Context, id string) error {
    if id == "" {
        return errors.New("id required")
    }

    data, err := fetchData(ctx, id)
    if err != nil {
        return fmt.Errorf("fetch data: %w", err)
    }

    return processData(ctx, data)
}

// Bad - nested conditionals
func Process(ctx context.Context, id string) error {
    if id != "" {
        data, err := fetchData(ctx, id)
        if err == nil {
            return processData(ctx, data)
        } else {
            return fmt.Errorf("fetch data: %w", err)
        }
    } else {
        return errors.New("id required")
    }
}

// Good - options pattern for many parameters
type ServerOptions struct {
    Port     int
    Timeout  time.Duration
    MaxConns int
}

func NewServer(opts ServerOptions) *Server { ... }

---

Testing Best Practices

Test Structure

• Table-driven tests: Use for testing multiple scenarios
• Test names: Descriptive, indicate what's being tested
• Setup/teardown: Use t.Cleanup() for resource cleanup
• Subtests: Use t.Run() to group related tests
• Test helpers: Accept *testing.T as first parameter
• Parallel tests: Use t.Parallel() when tests are independent

func TestUserValidation(t *testing.T) {
    tests := []struct {
        name    string
        user    User
        wantErr bool
    }{
        {
            name:    "valid user",
            user:    User{Name: "Alice", Email: "alice@example.com"},
            wantErr: false,
        },
        {
            name:    "missing name",
            user:    User{Email: "alice@example.com"},
            wantErr: true,
        },
        {
            name:    "invalid email",
            user:    User{Name: "Alice", Email: "not-an-email"},
            wantErr: true,
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            err := tt.user.Validate()
            if (err != nil) != tt.wantErr {
                t.Errorf("Validate() error = %v, wantErr %v", err, tt.wantErr)
            }
        })
    }
}

Test Coverage

• Critical paths: All critical business logic tested
• Error paths: Test error conditions, not just happy path
• Edge cases: Empty inputs, nil values, boundary conditions
• Integration tests: Test interactions between components
• Mock appropriately: Mock external dependencies, not internal logic
• Coverage target: Aim for >80%, but 100% is not always necessary

Test Quality

• Test isolation: Tests don't depend on execution order
• No test interdependence: Each test can run independently
• Deterministic: Tests produce same result every run
• Fast: Unit tests should be fast (< 1s), integration tests < 10s
• Clear failures: Error messages indicate what failed and why
• Avoid sleeps: Use synchronization, not time.Sleep() in tests

---

Common Pitfalls

1. Slice Append in Loops

// Potentially problematic - slice growth
for _, item := range hugeList {
    results = append(results, transform(item))
}

// Better - preallocate
results := make([]Result, 0, len(hugeList))
for _, item := range hugeList {
    results = append(results, transform(item))
}

2. Goroutine Leaks

// Bad - goroutine never terminates
func Process() {
    go func() {
        for {
            work() // Runs forever
        }
    }()
}

// Good - use context for cancellation
func Process(ctx context.Context) {
    go func() {
        for {
            select {
            case <-ctx.Done():
                return
            default:
                work()
            }
        }
    }()
}

3. Loop Variable Capture

// Bad - all goroutines see the last value
for _, item := range items {
    go func() {
        process(item) // BUG: item is the loop variable
    }()
}

// Good - pass as parameter or shadow
for _, item := range items {
    item := item // Shadow the loop variable
    go func() {
        process(item)
    }()
}

4. Context Not Passed

// Bad - context lost
func Handler(ctx context.Context) {
    doWork() // Context not propagated
}

// Good - pass context through
func Handler(ctx context.Context) {
    doWork(ctx)
}

5. Ignoring Close Errors

// Bad
defer file.Close()

// Good
defer func() {
    if err := file.Close(); err != nil {
        log.Printf("failed to close file: %v", err)
    }
}()

---

Documentation Standards

• Package docs: Every package has a package-level comment
• Exported symbols: All exported functions, types, constants documented
• Doc format: Complete sentences starting with the name
• Examples: Provide examples for complex APIs (Example_ functions)
• Links: Use godoc format for cross-references
• Deprecation: Mark deprecated items with Deprecated: comment

// Package user provides user management functionality.
package user

// User represents a system user with authentication credentials.
// It implements the Authenticator interface.
type User struct {
    ID       string
    Username string
    Email    string
}

// Validate checks if the user has all required fields.
// It returns an error if any required field is missing.
func (u *User) Validate() error {
    if u.Username == "" {
        return errors.New("username is required")
    }
    return nil
}

---

Review Checklist Template

Use this checklist when reviewing Go PRs:

Correctness

• Logic is correct and handles edge cases
• All errors are checked and handled appropriately
• No race conditions or data races
• Resource leaks prevented (defer cleanup)

Security

• No SQL/command injection vulnerabilities
• Secrets are not logged or exposed
• Input validation is performed
• No insecure cryptographic practices

Performance

• No unnecessary allocations or copies
• Database queries are efficient (no N+1)
• Appropriate use of caching if needed
• Context deadlines respected

Testing

• New code has adequate test coverage
• Tests cover error cases and edge cases
• Integration tests added if needed
• Tests are deterministic and fast

Code Quality

• Follows Go idioms and conventions
• Code is readable and maintainable
• Functions are focused and reasonably sized
• Proper error messages with context

Documentation

• All exported symbols are documented
• Complex logic has explanatory comments
• Breaking changes noted in PR description
• README/docs updated if needed

---

References

• Effective Go
• Go Code Review Comments
• Uber Go Style Guide
• Google Go Style Guide
• Go Proverbs