Code Review Best Practices for Engineering Teams

Code review is one of the highest-leverage practices in software engineering. A thorough review catches bugs before they reach production, spreads knowledge across the team, improves code consistency, and helps junior developers learn. But code review is only effective when done right — rushed reviews, massive pull requests, and personal criticism undermine the benefits. This article outlines best practices for both authors and reviewers.

Keep Pull Requests Small

The single most important factor in review quality is PR size. Studies show that code review effectiveness drops dramatically once a PR exceeds 400 lines of code. Small PRs (under 200 lines) are reviewed more thoroughly, catch more bugs, and ship faster. Break large features into a sequence of small, logically independent PRs — each one should add one coherent change. If you are refactoring, do not mix refactoring with feature work in the same PR. Use draft PRs for work-in-progress to get early design feedback without pressure. A good PR description explains what the change does, why it is needed, and how it was tested.

## Description
Add user authentication with JWT tokens

## Changes
- Add JWT token generation and validation
- Add login endpoint (POST /api/auth/login)
- Add token verification middleware to protected routes

## Testing
- [x] Unit tests for token generation and validation
- [x] Integration test for login flow
- [x] Test expired token rejection

## Related Issues
Closes #142

What to Look For in a Review

A good code review covers multiple dimensions. Correctness: does the code handle the requirements, including edge cases like empty states, null values, and error responses? Security: are there SQL injection vectors, XSS vulnerabilities, or hardcoded secrets? Performance: are there N+1 queries, unbounded list comprehensions, or obvious inefficiencies? Testability: are the functions testable in isolation, or are they tightly coupled to concrete dependencies? Readability: are the variable and function names descriptive? Is the control flow clear? Would a new team member understand this code? Focus on correctness and security first — style preferences are less important and can be enforced by automated formatters like Black, Ruff, or Prettier.

Automate Before Human Review

Set up CI to run linters, formatters, type checkers, and tests before a reviewer looks at the code. This frees human reviewers to focus on high-level concerns — design, correctness, and architecture — rather than nitpicking formatting or missing type annotations. Use a pre-commit configuration file so developers catch issues locally before pushing. GitHub Actions, GitLab CI, and Jenkins can all enforce these checks as required status checks that must pass before merging. A typical pre-commit config includes hooks for trailing whitespace, YAML validation, Python import sorting, and code formatting.

# .github/PULL_REQUEST_TEMPLATE.md
## Description
Briefly describe the change and why it is needed.

## Testing
- [ ] Unit tests pass
- [ ] Integration tests pass
- [ ] Manual testing performed

## Deployment Notes
Any migration steps, environment variables, or rollback considerations.

Checklists for Common Change Types

Different change types need different review focus. For a database migration PR, check for backward compatibility, rollback scripts, and performance impact on large tables. For a security-related change, look for input validation, authentication checks, and proper error handling that does not leak sensitive information. For a UI change, verify accessibility (keyboard navigation, screen reader support, color contrast according to WCAG 2.1 AA), loading states, and error messages. For an API change, ensure versioning is considered, deprecated fields are removed only after a transition period, and the OpenAPI spec is updated to reflect the changes.

The Review Process and Giving Feedback

A good review process has a clear workflow. The author creates a PR with a descriptive title and summary, checks CI passes, and assigns reviewers (typically 1-2 for simple changes, more for complex architectural decisions). Reviewers should respond within 24 hours — block time in your calendar for reviews just as you would for any other task. If a PR sits for days, context is lost and the author has to context-switch back to remember what they were doing. Use GitHub’s request changes, comment, and approve features appropriately. Frame feedback as questions rather than commands: instead of “Change this to use dependency injection,” say “Would dependency injection make this easier to test?” This invites discussion and acknowledges there may be context the reviewer does not have. Separate the code from the developer — critique the code, not the person. When receiving feedback, treat it as a learning opportunity. Not every comment needs to be addressed if there is a reasoned justification against it, but be open to changing your approach. If a reviewer does not understand your code, that is often a sign that the code needs better naming or documentation rather than a failing of the reviewer.

Test-Driven Development in Practice

Test-Driven Development (TDD) is a software development practice where you write tests before you write the production code. The cycle is simple but transformative: Red — write a failing test, Green — write the minimal code to make it pass, Refactor — clean up both the test and production code without changing behavior. This Red-Green-Refactor loop typically runs every few minutes, producing a steady cadence of small, validated increments. TDD leads to better-designed code, comprehensive test coverage, and a reliable safety net for refactoring.

The Red-Green-Refactor Cycle

Start by writing a test that describes the next behavior you want your code to have. The test should call an interface that does not exist yet (a function you have not written, a class you have not defined). Run the test — it fails (red), which confirms that the test is actually testing something. Now write the simplest possible production code to make the test pass. Do not worry about elegance or completeness — just make the test green. Once it passes (green), step back and refactor: remove duplication, rename variables, extract helper functions, improve the design. The tests stay green throughout refactoring because you are only changing structure, not behavior. Then start the next cycle with a new failing test.

import pytest
from calculator import Calculator

# Step 1: Write a failing test (RED)
def test_addition():
    calc = Calculator()
    result = calc.add(2, 3)
    assert result == 5

# Run: pytest -> FAILS because Calculator does not exist yet

# Step 2: Write minimal code to pass (GREEN)
class Calculator:
    def add(self, a, b):
        return a + b

# Run: pytest -> PASSES

Writing Testable Code

TDD naturally pushes you toward decoupled, testable code. When a test is hard to write, that is a signal that your design has problems — tight coupling, hidden dependencies, or unclear responsibilities. For example, if a function reads from a database or calls an external API, testing it directly would require setting up a real database connection or network access. Instead, inject dependencies as parameters so they can be replaced with test doubles (mocks, stubs, or fakes) during testing.

# Untestable — hard-coded dependency
def send_welcome_email(user_id):
    user = database.query(f"SELECT * FROM users WHERE id = {user_id}")
    smtp.send(user.email, "Welcome!", "Thanks for signing up!")

# Testable — dependency injection
def send_welcome_email(user_id, db, mailer):
    user = db.get_user(user_id)
    mailer.send(user.email, "Welcome!", "Thanks for signing up!")

# Now the test can pass in mocks
from unittest.mock import MagicMock

def test_send_welcome_email():
    mock_db = MagicMock()
    mock_db.get_user.return_value = type('User', (), {'email': 'test@example.com'})()
    mock_mailer = MagicMock()
    send_welcome_email(1, mock_db, mock_mailer)
    mock_mailer.send.assert_called_once_with(
        "test@example.com", "Welcome!", "Thanks for signing up!"
    )

Testing Edge Cases

Good tests cover not just the happy path but also edge cases — empty inputs, negative numbers, boundary values, nulls, duplicates, and error conditions. Each edge case should be a separate test with a descriptive name so that when a test fails, you immediately know what scenario broke. Parametrized tests let you run the same test logic with multiple inputs without duplicating code.

# Edge case tests for a divide function
def test_divide_positive():
    assert divide(10, 2) == 5

def test_divide_by_zero():
    with pytest.raises(ValueError, match="Cannot divide by zero"):
        divide(10, 0)

@pytest.mark.parametrize("a, b, expected", [
    (10, 2, 5), (0, 5, 0), (-6, 3, -2), (7, 3, 7/3),
])
def test_divide_parametrized(a, b, expected):
    assert divide(a, b) == expected

Test Fixtures and Setup

Fixtures handle repeated setup and teardown logic. In pytest, fixtures are functions decorated with @pytest.fixture that return objects or data needed by tests. Pytest manages fixture lifecycle — session-scoped fixtures are created once per test run, module-scoped once per module, and function-scoped (the default) for each test. Use fixtures to create test databases, load sample data, set up configuration, or instantiate complex objects.

import pytest, tempfile, os

@pytest.fixture
def calculator():
    return Calculator()

@pytest.fixture
def temp_data_file():
    with tempfile.NamedTemporaryFile(mode='w', suffix='.csv', delete=False) as f:
        f.write("name,age\nAlice,30\nBob,25\n")
        path = f.name
    yield path
    os.unlink(path)

def test_calculator_add(calculator):
    assert calculator.add(2, 3) == 5

def test_load_csv(temp_data_file):
    data = load_csv(temp_data_file)
    assert len(data) == 2

Mocking External Dependencies

When your code interacts with external services (APIs, databases, file systems), mocking lets you test the behavior without the real dependency. Python’s unittest.mock library provides Mock and patch for replacing objects during testing. Use patch as a context manager or decorator to temporarily replace a function or class with a mock that records how it was called and returns configured values.

from unittest.mock import patch

@patch('myapp.mailer.send')
def test_registration_sends_email(mock_send):
    register_user("alice@example.com")
    mock_send.assert_called_once()

# Mocking external API calls
from unittest.mock import MagicMock

@patch('requests.get')
def test_fetch_user(mock_get):
    mock_response = MagicMock()
    mock_response.json.return_value = {"id": 1, "name": "Alice"}
    mock_response.status_code = 200
    mock_get.return_value = mock_response
    result = fetch_user(1)
    assert result["name"] == "Alice"

TDD is a discipline that takes practice. The first few weeks feel slower because you are writing tests before code, but the speed compounds quickly — you spend far less time manually testing, debugging regressions, and fixing bugs that reach production. Teams that adopt TDD consistently report higher code quality, fewer production incidents, and greater confidence when refactoring or adding features.

API Versioning Strategies for Long-Term Projects

API versioning is essential for any public or internal API that evolves over time. Without a versioning strategy, changing an endpoint’s behavior will break existing clients. A well-designed versioning approach lets you add features, fix bugs, and improve performance without disrupting users who depend on the current contract. This article covers the main versioning strategies — URL, header, and query parameter — along with deprecation practices and migration patterns.

URL Path Versioning

The most common and simplest approach is to include the version number in the URL path, such as /api/v1/users and /api/v2/users. This makes the version explicit, easy to route at the web server level (Nginx, API gateway), and straightforward for clients to understand and configure. The main downside is URL bloat — every endpoint URL changes when you bump the version, which can be annoying for clients that hardcode URLs. URL versioning also makes it tempting to create a new version for every small change, which leads to many underused API versions that must all be maintained.

# URL versioning — best for public APIs with many consumers
GET /api/v1/users
POST /api/v1/users
GET /api/v2/users  # new version with breaking changes

Header Versioning

Header versioning keeps the URL clean by specifying the version in a custom HTTP header or the Accept header using a media type parameter. This approach keeps URLs stable — /api/users always works — but makes version discovery harder for developers because the version is not visible in the URL or in documentation examples. It also adds complexity to client setup since custom headers must be configured. API gateways and proxies may also strip or modify custom headers, which can cause unexpected routing.

# Accept header versioning (media type)
GET /api/users
Accept: application/vnd.myapp.v1+json

# Custom header versioning
GET /api/users
X-API-Version: 1

# Response with deprecation headers
HTTP/1.1 200 OK
Sunset: Sat, 01 Nov 2027 00:00:00 GMT
Deprecation: true
Link: ; rel="successor-version"

The Sunset header tells clients when the old version will be removed, the Deprecation header signals that this version is deprecated, and the Link header with rel="successor-version" points to the replacement. These headers give clients a clear migration timeline without requiring them to check external documentation or changelogs.

Query Parameter Versioning

Query parameter versioning appends the version as a query string: /api/users?version=1. This is the easiest to implement on the server side (just read a query parameter) but has significant drawbacks. Query parameters are often ignored by caching layers, so different versions of the same resource are not cached separately. They also clutter API logs and URLs, and clients can accidentally omit the parameter entirely, causing unexpected behavior from the default version.

Backward-Compatible Changes

Before creating a new API version, consider whether the change can be made backward-compatible. Adding new optional fields to a response, adding new endpoints, or making previously required fields optional are all safe changes that do not require a version bump. The guideline is: be liberal in what you accept and conservative in what you send. Always include fields that clients might depend on rather than removing them, and use null or sensible defaults for new optional fields so existing parsers do not break.

# Backward-compatible: add new fields to response
{
  "id": 42,
  "name": "Alice",
  "email": "alice@example.com",
  "created_at": "2026-01-15T10:00:00Z",
  "profile_url": null        # new field, null by default
}

# Backward-compatible: make previously required field optional
# Old: {"username": "alice"} — username is required
# New: {"username": "alice", "email": "alice@example.com"} — username still works

Deprecation and Migration

When a breaking change is unavoidable, deprecate the old version with a clear timeline. Support each version for 6-12 months after announcing deprecation. Communicate the deprecation through multiple channels: deprecation headers in API responses, email notifications to registered developers, changelog entries, and documentation banners. Provide a migration guide that explains what changed and how to update client code. After the sunset date, return HTTP 410 Gone for deprecated endpoints rather than silently failing — this gives a clear signal to clients that the endpoint is no longer available.

# OpenAPI deprecation marker
paths:
  /api/v1/users:
    get:
      deprecated: true
      summary: "List users (deprecated — use /api/v2/users)"
      responses:
        '200':
          description: "User list"

# Server-side version router (Python example)
from fastapi import APIRouter

v1_router = APIRouter(prefix="/api/v1")
v2_router = APIRouter(prefix="/api/v2")

@v1_router.get("/users")
def list_users_v1():
    return [{"id": 1, "name": "Alice"}]  # old schema

@v2_router.get("/users")
def list_users_v2():
    return [{"id": 1, "name": "Alice", "email": "alice@example.com"}]

The best versioning strategy depends on your audience. Public APIs with many external consumers benefit from URL versioning’s explicitness. Internal APIs within a single organization can use header versioning for cleaner URLs. Whichever strategy you choose, minimize the number of versions you maintain — ideally no more than two at a time — and automate the deprecation process so that sunset dates are enforced consistently.