Code Review Practices: Guidelines for Effective Code Reviews

# Good feedback examples

## Suggestion (not demand)
"Consider using a dictionary here for O(1) lookups instead of the current list iteration. 
This would improve performance from O(n) to O(1) for the lookup operation."

## Question (not accusation)
"What's the reasoning behind using recursion here? I'm wondering if an iterative 
approach might be simpler and avoid potential stack overflow for large inputs."

## Explanation (with rationale)
"This pattern could be confusing because it mixes business logic with data access. 
Using the repository pattern would make it clearer by separating concerns and 
making the code more testable."

## Praise (when deserved)
"Nice solution! The error handling here is very thorough and the error messages 
are actionable. This will make debugging much easier."

## Offering alternatives
"Instead of manually parsing the JSON, we could use Pydantic models which would 
give us automatic validation and better type safety. Here's an example: [link]"

## Asking for clarification
"I'm not sure I understand the purpose of this function. Could you add a docstring 
explaining what it does and when it should be used?"

# Avoid

- "Change this" (imperative without explanation)
- "Wrong" (judgmental without context)
- "Why did you..." (accusatory tone)
- "This is bad" (unhelpful criticism)
- "Everyone knows..." (condescending)
- "Just use X" (dismissive of author's approach)

## Blocking (must fix before merge)
**Blocking**: The database connection isn't closed in the error path. This will 
cause connection leaks in production.

## Major (should fix, but not blocking)
**Major**: This function has cyclomatic complexity of 25. Consider breaking it 
into smaller functions for maintainability.

## Minor (nice to have)
**Minor**: Consider extracting this magic number (86400) into a named constant 
like SECONDS_PER_DAY for clarity.

## Nit (style/preference, non-blocking)
**Nit**: Personal preference, but I find this more readable with the condition 
on a separate line.

## Question (seeking understanding)
**Question**: Why are we using a custom implementation instead of the standard 
library function? Is there a performance reason?

## Praise (positive feedback)
**Praise**: Excellent use of the strategy pattern here. This makes adding new 
payment methods trivial.

## Code Review: Add Payment Processing Feature

### Summary
**Reviewed by**: @reviewer
**Files changed**: 12
**Lines added**: 450
**Lines removed**: 80
**Review time**: 45 minutes

### What Works Well
- Clean separation between payment gateway adapters
- Comprehensive test coverage (92%)
- Clear error messages with actionable guidance
- Good use of dependency injection for testability
- Proper logging at appropriate levels

### Suggestions

#### Major
1. **Payment retry logic**: The retry mechanism doesn't implement exponential backoff. 
   This could overwhelm the payment gateway during outages. Consider using a library 
   like `tenacity` or implementing exponential backoff manually.

2. **Transaction atomicity**: The order status update and payment record creation 
   aren't in a database transaction. If one fails, we'll have inconsistent state.

#### Minor
3. **Error handling**: Consider creating custom exception types (PaymentDeclinedError, 
   PaymentTimeoutError) instead of generic exceptions. This makes error handling 
   more precise.

4. **Configuration**: The payment gateway URLs are hardcoded. Move these to 
   configuration for easier environment management.

5. **Documentation**: Add docstrings to the public methods explaining parameters 
   and return values.

### Concerns

**Blocking**: Line 145 - The API key is logged in the error message. This is a 
security vulnerability. Remove sensitive data from logs.

**Blocking**: Line 203 - SQL query is vulnerable to injection. Use parameterized 
queries instead of string formatting.

### Questions

1. Why did we choose to implement our own retry logic instead of using the payment 
   gateway's built-in retry mechanism?

2. What's the expected behavior if the payment succeeds but the webhook notification 
   fails? Do we have a reconciliation process?

### Approval Status
⏸️ **Changes Requested** - Please address the two blocking issues, then I'll approve.

### Next Steps
1. Fix security issues (API key logging, SQL injection)
2. Add transaction wrapper for atomicity
3. Consider implementing exponential backoff
4. Update once fixed and I'll re-review

# Bug: Off-by-one error
for i in range(len(items) - 1):  # Wrong: misses last item
    process(items[i])

# Fixed
for i in range(len(items)):
    process(items[i])

# Better: Pythonic iteration
for item in items:
    process(item)

# Bug: Mutable default argument
def add_item(item, items=[]):  # Wrong: shared across calls
    items.append(item)
    return items

# Fixed
def add_item(item, items=None):
    if items is None:
        items = []
    items.append(item)
    return items

# Bug: Incorrect boolean logic
if user.is_active and not user.is_banned or user.is_admin:  # Ambiguous!
    allow_access()

# Fixed: Use parentheses for clarity
if (user.is_active and not user.is_banned) or user.is_admin:
    allow_access()

# Bug: Race condition
if not file_exists(path):
    create_file(path)  # Another process might create it between check and create

# Fixed: Handle the race
try:
    create_file(path)
except FileExistsError:
    pass  # File was created by another process, that's fine

# Bug: SQL injection
query = f"SELECT * FROM users WHERE username = '{username}'"  # Dangerous!
db.execute(query)

# Fixed: Parameterized query
query = "SELECT * FROM users WHERE username = ?"
db.execute(query, (username,))

# Bug: Logging sensitive data
logger.info(f"User logged in: {username}, password: {password}")  # Never log passwords!

# Fixed
logger.info(f"User logged in: {username}")

# Bug: Hardcoded secret
API_KEY = "sk-1234567890abcdef"  # Never commit secrets!

# Fixed
API_KEY = os.getenv("API_KEY")
if not API_KEY:
    raise ValueError("API_KEY environment variable is required")

# Bug: No input validation
def transfer_money(amount):
    account.balance -= amount  # What if amount is negative?

# Fixed
def transfer_money(amount):
    if amount <= 0:
        raise ValueError("Amount must be positive")
    if amount > account.balance:
        raise InsufficientFundsError()
    account.balance -= amount

# Bug: N+1 query problem
users = db.query("SELECT * FROM users")
for user in users:
    posts = db.query(
        "SELECT * FROM posts WHERE user_id = ?", user.id
    )  # Query per user!

# Fixed: Eager loading or JOIN
users = db.query("""
    SELECT u.*, p.*
    FROM users u
    LEFT JOIN posts p ON u.id = p.user_id
""")

# Bug: Inefficient algorithm
def find_duplicates(items):
    duplicates = []
    for i, item in enumerate(items):
        for j, other in enumerate(items):
            if i != j and item == other:
                duplicates.append(item)  # O(n²)
    return duplicates

# Fixed: Use set for O(n)
def find_duplicates(items):
    seen = set()
    duplicates = set()
    for item in items:
        if item in seen:
            duplicates.add(item)
        seen.add(item)
    return list(duplicates)

# Bug: Loading entire dataset into memory
def process_large_file(filename):
    data = open(filename).read()  # Loads entire file!
    for line in data.split('\n'):
        process(line)

# Fixed: Stream processing
def process_large_file(filename):
    with open(filename) as f:
        for line in f:  # Reads line by line
            process(line)

# Bug: Test depends on execution order
class TestUser:
    def test_create_user(self):
        user = User.create("[email protected]")
        assert user.id == 1  # Assumes this runs first!
    
    def test_delete_user(self):
        user = User.get(1)  # Depends on previous test!
        user.delete()

# Fixed: Independent tests
class TestUser:
    def test_create_user(self):
        user = User.create("[email protected]")
        assert user.id is not None
        user.delete()  # Clean up
    
    def test_delete_user(self):
        user = User.create("[email protected]")  # Create own data
        user_id = user.id
        user.delete()
        assert User.get(user_id) is None

# Bug: Testing implementation instead of behavior
def test_calculate_total(self):
    cart = Cart()
    cart.items = [Item(price=10), Item(price=20)]  # Accessing internals!
    assert cart._calculate_subtotal() == 30  # Testing private method!

# Fixed: Test public interface
def test_calculate_total(self):
    cart = Cart()
    cart.add_item(Item(price=10))
    cart.add_item(Item(price=20))
    assert cart.total == 30  # Test public behavior

# Python
black .                    # Code formatter
flake8 .                   # Style checker
pylint src/                # Static analyzer
mypy src/                  # Type checker

# JavaScript/TypeScript
eslint src/                # Linter
prettier --write src/      # Formatter
tsc --noEmit              # Type checker

# Go
gofmt -w .                # Formatter
golint ./...              # Linter
go vet ./...              # Static analyzer

# Rust
cargo fmt                 # Formatter
cargo clippy              # Linter

# Dependency vulnerabilities
npm audit                 # Node.js
pip-audit                 # Python
cargo audit               # Rust

# Static security analysis
bandit -r src/            # Python security
semgrep --config=auto .   # Multi-language
snyk test                 # Dependency scanning

# Secret detection
gitleaks detect           # Find committed secrets
trufflehog git file://.   # Secret scanner

# Complexity analysis
radon cc src/ -a          # Python cyclomatic complexity
lizard src/               # Multi-language complexity

# Test coverage
pytest --cov=src tests/   # Python
jest --coverage           # JavaScript
go test -cover ./...      # Go

# Code duplication
jscpd src/                # Multi-language

# GitHub Actions example
name: Code Review Checks

on: [pull_request]

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - name: Run linters
        run: |
          black --check .
          flake8 .
          mypy src/
  
  test:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - name: Run tests
        run: |
          pytest --cov=src --cov-report=xml
      - name: Upload coverage
        uses: codecov/codecov-action@v3
  
  security:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - name: Security scan
        run: |
          bandit -r src/
          pip-audit
          gitleaks detect

For complex or critical changes, consider pair programming instead of async review:

**Benefits**:
- Immediate feedback
- Knowledge transfer in real-time
- Faster iteration
- Builds relationships

**When to use**:
- Complex algorithms
- Critical security code
- Architectural changes
- Onboarding new team members

For architectural decisions or major refactorings, gather the team for a mob review:

**Process**:
1. Author presents the change (10 min)
2. Team reviews code together (30 min)
3. Discussion and decision (20 min)

**Benefits**:
- Entire team understands the change
- Faster consensus on controversial decisions
- Shared ownership

# Python-specific checklist
- [ ] Type hints on function signatures
- [ ] Docstrings follow Google/NumPy style
- [ ] No mutable default arguments
- [ ] Context managers for resource management
- [ ] List comprehensions for simple transformations
- [ ] f-strings for string formatting
- [ ] Pathlib for file operations
- [ ] Dataclasses for data containers

// JavaScript-specific checklist
- [ ] Const/let instead of var
- [ ] Arrow functions for callbacks
- [ ] Async/await instead of callbacks
- [ ] Destructuring for object/array access
- [ ] Template literals for strings
- [ ] Optional chaining (?.) for nested access
- [ ] Nullish coalescing (??) for defaults
- [ ] No == (use ===)

// Go-specific checklist
- [ ] Error handling on every error return
- [ ] Defer for cleanup (close, unlock)
- [ ] Context for cancellation
- [ ] Goroutine leaks prevented
- [ ] Race conditions checked (go run -race)
- [ ] Exported functions have comments
- [ ] Interfaces are small and focused