Go code review checklist: a hands-on guide for engineers

Go code review checklist: a hands-on guide for engineers

Code review is where bugs die before they reach production. In Go, a thorough review catches resource leaks, unhandled errors, and architectural missteps that will haunt your team later. This guide walks you through the checklist every senior engineer should use when reviewing Go code for production APIs.

Why code review matters in Go

Go's simplicity is a double-edged sword. The language doesn't force you to think about goroutines, context propagation, or error wrapping—which means reviewers must. A single forgotten defer cancel() in a high-traffic API endpoint can leak goroutines and crash your service weeks later. Production experience teaches us that the cheapest fix is the one that never ships.

Errors handled and wrapped

Error handling is the first thing we look for. In Go, every function that can fail returns an error. The question isn't whether you handled it—it's whether you handled it correctly.

What we check:

• No naked return statements that discard errors.
• Errors are wrapped with context using fmt.Errorf with %w.
• Sentinel errors or type assertions are used where callers need to distinguish failure modes.
• Timeouts and cancellations are handled as separate cases from logic errors.

Here's what we often see in code review:

// Bad: error discarded
func FetchUser(ctx context.Context, id string) *User {
    user, _ := db.Query(ctx, id)
    return user
}

// Better: error checked but no context
func FetchUser(ctx context.Context, id string) (*User, error) {
    user, err := db.Query(ctx, id)
    if err != nil {
        return nil, err
    }
    return user, nil
}

// Best: error wrapped with context
func FetchUser(ctx context.Context, id string) (*User, error) {
    user, err := db.Query(ctx, id)
    if err != nil {
        return nil, fmt.Errorf("fetch user %s: %w", id, err)
    }
    return user, nil
}

Error wrapping matters because it builds a chain that tells the story of what went wrong. When your monitoring system logs that error six layers deep in the call stack, you need to know whether it started as a network timeout, a database constraint violation, or a client sending bad data. Wrapped errors let you do that.

We also watch for error handling that swallows important information. A common mistake is converting errors to strings early:

// Problematic: error type lost
func ProcessPayment(ctx context.Context, id string) error {
    err := chargeCard(ctx, id)
    if err != nil {
        return fmt.Errorf("payment failed: %s", err.Error())
    }
    return nil
}

// Better: preserve error type with %w
func ProcessPayment(ctx context.Context, id string) error {
    err := chargeCard(ctx, id)
    if err != nil {
        return fmt.Errorf("payment failed: %w", err)
    }
    return nil
}

With %w, callers can use errors.Is() and errors.As() to inspect the underlying error. That matters when your payment gateway returns a specific error type that tells you the card was declined versus the API was unreachable.

Contexts passed through

Context is how Go coordinates cancellation and timeouts across goroutines and service boundaries. Every function that does I/O—database calls, HTTP requests, pub/sub operations—must accept context.Context as its first parameter. We check this rigorously.

What we check:

• All I/O-bound functions take ctx context.Context as the first argument.
• Context is never stored in structs; it's passed as arguments.
• Context is checked before expensive operations via ctx.Done().
• Timeouts and deadlines are set at the API boundary, not buried in library code.
• No context.Background() or context.TODO() in production code paths (only in main or tests).

Here's a common pattern in production APIs:

// Correct: context flows through
func (h *Handler) CreateOrder(w http.ResponseWriter, r *http.Request) {
    ctx, cancel := context.WithTimeout(r.Context(), 5*time.Second)
    defer cancel()
    
    order, err := h.service.CreateOrder(ctx, req)
    if err != nil {
        if errors.Is(err, context.DeadlineExceeded) {
            http.Error(w, "timeout", http.StatusGatewayTimeout)
            return
        }
        http.Error(w, err.Error(), http.StatusInternalServerError)
        return
    }
    // handle success
}

// Service uses the context
func (s *Service) CreateOrder(ctx context.Context, req *OrderReq) (*Order, error) {
    // Check context before starting work
    if err := ctx.Err(); err != nil {
        return nil, err
    }
    
    order := &Order{ID: uuid.New().String()}
    
    // Pass context to database operation
    if err := s.db.Insert(ctx, order); err != nil {
        return nil, fmt.Errorf("insert order: %w", err)
    }
    
    return order, nil
}

A mistake we catch repeatedly is storing context in a struct or goroutine scope when the caller no longer needs it:

// Bad: context stored, outlives caller intent
type Worker struct {
    ctx context.Context
    queue chan Job
}

func (w *Worker) Process(job Job) {
    // Using w.ctx here can continue long after caller cancels
}

// Good: context passed explicitly
func (w *Worker) Process(ctx context.Context, job Job) {
    // Respects caller's cancellation immediately
}

No goroutine leaks

Goroutine leaks are silent killers. A service that spawns goroutines without cleanup will consume memory until it crashes. We look for these patterns in every review.

What we check:

• Every go func() can be stopped or has clear termination logic.
• Channels are closed by the sender, and receivers respect cancellation.
• Background workers use context cancellation or explicit shutdown signals.
• No infinite loops without context checks.
• Buffered channels have bounded capacity.

Here's a leaky pattern we see in handler code:

// Leaky: goroutine waits forever
func (h *Handler) NotifyUser(w http.ResponseWriter, r *http.Request) {
    userID := r.URL.Query().Get("id")
    
    go func() {
        time.Sleep(time.Hour)
        sendEmail(userID)
    }()
    
    w.WriteHeader(http.StatusAccepted)
}

// Better: respects context
func (h *Handler) NotifyUser(w http.ResponseWriter, r *http.Request) {
    userID := r.URL.Query().Get("id")
    
    go func() {
        select {
        case <-h.ctx.Done():
            return
        case <-time.After(time.Hour):
            sendEmail(userID)
        }
    }()
    
    w.WriteHeader(http.StatusAccepted)
}

// Best: explicit worker pool with shutdown
func (s *Service) NotifyUser(ctx context.Context, userID string) {
    s.notifyQueue <- ¬ification{userID: userID}
}

func (s *Service) Start(ctx context.Context) {
    go func() {
        for {
            select {
            case <-ctx.Done():
                return
            case notif := <-s.notifyQueue:
                sendEmail(notif.userID)
            }
        }
    }()
}

The second example is better but still has a problem: if the service shuts down before that hour expires, the goroutine is abandoned. The third pattern—a proper worker—ensures cleanup happens in a controlled way.

Clear package names

Package organization affects readability, testability, and API design. We check that packages have single, clear responsibilities and names that reflect them.

What we check:

• Package names describe what they do, not the project name.
• No package named utils, helpers, or common that becomes a dumping ground.
• Internal structure is private (internal/ directory) when appropriate.
• Public interfaces are minimal and well-named.
• No circular dependencies.

Weak package structure:

// myapp/utils/utils.go - a red flag
package utils

func IsValidEmail(s string) bool { ... }
func IsValidPhone(s string) bool { ... }
func ParseDate(s string) (time.Time, error) { ... }

Clear structure:

// myapp/validation/email.go
package validation

func Email(s string) error { ... }

// myapp/validation/phone.go
package validation

func Phone(s string) error { ... }

// myapp/time/parser.go
package time

func Parse(s string) (time.Time, error) { ... }

The second approach makes dependencies explicit. When someone imports validation, they know they're validating input. When they import time, it's clear they're parsing dates.

Common mistakes

• Ignoring errors silently: _ = operation() hides bugs. At minimum, log with logrus.WithError(err).
• Using context.Background() in handlers: Always use the request context or a derivative with a timeout.
• Spawning goroutines without lifecycle management: Every goroutine needs a way to shut down gracefully.
• Package names that don't describe responsibility: pkg, utils, and helpers are warning signs of muddled design.
• Wrapping errors without context: return fmt.Errorf("error: %v", err) loses caller information. Use %w and include the operation.
• Not checking context before I/O: A cancelled context will fail the I/O anyway, but checking early saves CPU.

Key takeaways

• Error wrapping with %w preserves error types and builds diagnostic chains that matter in production.
• Context must flow through every I/O-bound function as the first argument, never stored in structs.
• Goroutines without cancellation or termination logic are memory leaks waiting to happen; every go statement needs an exit path.
• Package names should describe responsibility, not be generic labels like utils or common.
• Code review is the fastest way to catch these issues before they become production incidents; invest time in checklists.