Code Review Best Practices

Production engineering guide for code review best practices covering patterns, implementation strategies, and operational best practices.

This page describes specific techniques and patterns for conducting effective code reviews. These practices apply when engineers review changes before merging to mainline branches, particularly in systems where multiple people contribute to shared codebases.

Understand What You're Reviewing

Before beginning a review, examine the commit history and understand what changes are being introduced. Use git log with specific flags to see recent changes:

git log --oneline -10
git show --stat <commit-hash>

When reviewing, focus on whether the change addresses the intended problem, not just the implementation details. A common failure is accepting code that works but doesn't solve the actual issue.

Check for Correctness

Verify that code behaves as expected by examining test coverage and edge cases. Run existing tests before approving:

make test
# or
go test ./...

Look for missing error handling, particularly in functions that return errors. For example, a function that returns an error but doesn't check it:

func processFile(name string) error {
    file, err := os.Open(name)
    if err != nil {
        return err
    }
    defer file.Close()
    // missing: file.Close() error check
    return nil
}

Evaluate Readability and Maintainability

Code should be readable without requiring extensive context. If a reviewer needs to ask questions during review, that's a sign of poor readability.

Avoid overly clever patterns unless they're well-documented:

// Good: clear function name with descriptive parameters
func calculateDiscount(price float64, discountPercent float64) float64 {
    return price * (1 - discountPercent/100)
}

// Avoid: cryptic variable names
func calc(x, y float64) float64 {
    return x * (1 - y/100)
}

Verify API Contract Compliance

When modifying APIs, ensure changes don't break existing contracts. Check that all exported functions maintain their signatures:

// Before
func ProcessOrder(orderID string) (*Order, error)

// After should maintain same signature if contract is stable
func ProcessOrder(orderID string) (*Order, error)

Look for Resource Management Issues

Resource leaks are common in code reviews. Check that resources like files, database connections and network sockets are properly closed:

func readConfig(filename string) ([]byte, error) {
    file, err := os.Open(filename)
    if err != nil {
        return nil, err
    }
    defer file.Close()
    
    // Missing: check file.Close() error
    return io.ReadAll(file)
}

Identify Potential Performance Issues

Look for inefficient patterns that could cause problems under load. For example, avoid calling expensive operations in loops:

// Bad: O(n²) complexity
func findMatches(items []string, patterns []string) []string {
    var result []string
    for _, item := range items {
        for _, pattern := range patterns {
            if strings.Contains(item, pattern) {
                result = append(result, item)
                break
            }
        }
    }
    return result
}

// Better: O(n) with map lookup
func findMatches(items []string, patterns []string) []string {
    patternMap := make(map[string]bool)
    for _, pattern := range patterns {
        patternMap[pattern] = true
    }
    
    var result []string
    for _, item := range items {
        for pattern := range patternMap {
            if strings.Contains(item, pattern) {
                result = append(result, item)
                break
            }
        }
    }
    return result
}

Check for Security Vulnerabilities

Look for common security issues in code changes:

For example, a vulnerable SQL query:

func getUser(username string) (*User, error) {
    query := "SELECT * FROM users WHERE username = '" + username + "'"
    // missing: parameterized queries
    rows, err := db.Query(query)
    if err != nil {
        return nil, err
    }
    defer rows.Close()
    // ...
}

Review Configuration and Environment Handling

Check that code handles configuration changes properly. Ensure environment variables are used consistently:

// Good: explicit environment variable usage
func getDatabaseURL() string {
    url := os.Getenv("DATABASE_URL")
    if url == "" {
        return "postgres://localhost:5432/myapp"
    }
    return url
}

Verify Error Handling Patterns

Ensure errors are handled consistently throughout the codebase. Missing error checks can cause silent failures:

// Bad: ignoring errors
func processRequest(req *http.Request) error {
    body, _ := io.ReadAll(req.Body)
    defer req.Body.Close()
    
    // Missing: check body read error
    var data map[string]interface{}
    json.Unmarshal(body, &data) // ignoring unmarshal error
    return nil
}

// Good: proper error handling
func processRequest(req *http.Request) error {
    body, err := io.ReadAll(req.Body)
    if err != nil {
        return fmt.Errorf("read request body: %w", err)
    }
    defer req.Body.Close()
    
    var data map[string]interface{}
    if err := json.Unmarshal(body, &data); err != nil {
        return fmt.Errorf("parse JSON: %w", err)
    }
    return nil
}

Examine Test Coverage and Quality

Review that tests cover the new functionality appropriately:

// Example test for a function
func TestCalculateDiscount(t *testing.T) {
    tests := []struct {
        name     string
        price    float64
        discount float64
        want     float64
    }{
        {"No discount", 100.0, 0.0, 100.0},
        {"Full discount", 100.0, 100.0, 0.0},
        {"Partial discount", 100.0, 25.0, 75.0},
    }
    
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            got := calculateDiscount(tt.price, tt.discount)
            if got != tt.want {
                t.Errorf("calculateDiscount(%v, %v) = %v, want %v", 
                    tt.price, tt.discount, got, tt.want)
            }
        })
    }
}

Look for Logic Errors

Check that business logic is implemented correctly. For example, ensure conditionals have the right operators:

// Bad: logical error with OR instead of AND
func canAccess(user *User, resource *Resource) bool {
    return user.Role == "admin" || user.CanRead(resource)
}

// Good: correct logic
func canAccess(user *User, resource *Resource) bool {
    return user.Role == "admin" || user.CanRead(resource)
}

Evaluate Logging and Monitoring

Check that code includes appropriate logging for debugging:

func processPayment(amount float64) error {
    log.Printf("Processing payment of %.2f", amount)
    
    // missing: log errors or return values that might be important
    err := chargeCard(amount)
    if err != nil {
        log.Printf("Payment failed for amount %.2f: %v", amount, err)
        return err
    }
    
    log.Printf("Payment successful for amount %.2f", amount)
    return nil
}

Validate Input Validation

Ensure that input validation is comprehensive:

// Good: validate inputs
func createUser(name string, age int) (*User, error) {
    if name == "" {
        return nil, fmt.Errorf("name cannot be empty")
    }
    if age < 0 || age > 150 {
        return nil, fmt.Errorf("age must be between 0 and 150")
    }
    
    return &User{Name: name, Age: age}, nil
}

Check for Data Race Conditions

In concurrent code, verify that shared data is properly protected:

// Bad: potential race condition
type Counter struct {
    count int
}

func (c *Counter) Increment() {
    c.count++ // not safe in concurrent usage
}

// Good: use sync.Mutex
type Counter struct {
    count int
    mu    sync.Mutex
}

func (c *Counter) Increment() {
    c.mu.Lock()
    defer c.mu.Unlock()
    c.count++
}

Consider Side Effects and State Changes

Review functions that have side effects or modify global state:

// Bad: function with unexpected side effect
func processOrder(orderID string) error {
    // modifies global configuration
    config.DatabaseURL = "new-url"
    return nil
}

// Good: explicit side effect handling
func processOrder(orderID string, config *Config) error {
    // only modify provided config
    config.DatabaseURL = "new-url"
    return nil
}

Review Documentation and Comments

Ensure code changes include appropriate documentation:

// Bad: no comments
func calculateTax(amount float64, rate float64) float64 {
    return amount * rate
}

// Good: clear documentation
// CalculateTax returns the tax amount for a given amount and rate.
// Rate should be expressed as a decimal (e.g., 0.08 for 8%).
func calculateTax(amount float64, rate float64) float64 {
    return amount * rate
}

Check for Consistent Naming Conventions

Verify that code follows established naming conventions in the project:

// Good: consistent naming
type UserService struct {
    db *sql.DB
}

func (s *UserService) FindUser(id int) (*User, error) {
    // implementation
}

// Bad: inconsistent naming
type userService struct {
    database *sql.DB
}

func (s *userService) find_user(id int) (*User, error) {
    // implementation
}

Validate Dependencies and Imports

Ensure that code doesn't introduce unnecessary dependencies:

// Good: only import what's needed
import (
    "fmt"
    "net/http"
)

// Bad: importing unused packages
import (
    "fmt"
    "net/http"
    "os"
    "time"
    "math"
)

Examine Commit Message Quality

Review that commit messages clearly explain what changed and why:

# Good commit message
git commit -m "Fix null pointer dereference in user service
- Added nil check for user object
- Added test case for empty user scenario
- Fixes #1234"

Handle Review Feedback

When responding to review comments, ensure that changes are clear and complete:

# After addressing feedback
git commit --amend -m "Fix null pointer dereference in user service
- Added nil check for user object
- Added test case for empty user scenario
- Fixes #1234"

Common Review Pitfalls

A common mistake is approving code that doesn't fully address the problem. Reviewers should ensure:

  1. The change solves the stated problem
  2. All edge cases are handled
  3. Error conditions are properly managed
  4. Performance characteristics are acceptable
  5. Security implications are considered
  6. The code integrates well with existing patterns

Silent failures occur when reviewers approve code that compiles but doesn't behave correctly in production. This often happens with:

The review process should be iterative, not a one-time event. Code reviews are most effective when they happen early and often, ideally as part of a continuous integration workflow where reviewers can see the impact of changes before they merge to mainline branches.

This page was rewritten on 15 October 2026. It replaced a templated version that was not specific to its own title. How these pages are written and checked. If anything here is wrong, tell us at [email protected] and we will correct it.