Code Review Best Practices: Write Reviews That Actually Improve Code

/**
 * A realistic example of the kind of bug a good code reviewer should catch.
 * The author's intent is clear. The tests might even pass. But there's a
 * correctness bug hiding in plain sight.
 */
public class OrderDiscountService {

    private static final double LOYALTY_DISCOUNT_RATE = 0.10;  // 10% for loyal customers
    private static final int    LOYALTY_THRESHOLD_DAYS = 365;  // 1 year to qualify

    /**
     * Calculates the final price after applying a loyalty discount.
     * BUG: uses integer division — if orderTotal is less than 10,
     * the discount rounds down to 0, silently giving no discount at all.
     */
    public double calculateLoyaltyPrice(double orderTotal, int customerAgeDays) {

        if (customerAgeDays < LOYALTY_THRESHOLD_DAYS) {
            // Customer hasn't been with us a year yet — no discount
            return orderTotal;
        }

        // REVIEWER SHOULD CATCH THIS: casting to int before multiplying
        // truncates the discount for small orders, e.g. $9.99 * 0.10 = $0.999,
        // cast to int = 0, so discount becomes $0.00
        int discountAmount = (int) (orderTotal * LOYALTY_DISCOUNT_RATE);

        return orderTotal - discountAmount;
    }

    /**
     * FIXED version a reviewer would suggest:
     * Keep the calculation in floating point throughout.
     */
    public double calculateLoyaltyPriceFixed(double orderTotal, int customerAgeDays) {

        if (customerAgeDays < LOYALTY_THRESHOLD_DAYS) {
            return orderTotal;
        }

        // Stay in double — no silent truncation
        double discountAmount = orderTotal * LOYALTY_DISCOUNT_RATE;

        return orderTotal - discountAmount;
    }

    public static void main(String[] args) {

        OrderDiscountService service = new OrderDiscountService();

        double smallOrder = 9.50;   // A customer buys a $9.50 item
        int loyalCustomerAge = 400; // They've been with us 400 days — qualifies

        double buggyResult = service.calculateLoyaltyPrice(smallOrder, loyalCustomerAge);
        double fixedResult  = service.calculateLoyaltyPriceFixed(smallOrder, loyalCustomerAge);

        // Show the reviewer the cost of missing this bug
        System.out.println("=== Loyalty Discount Comparison ===");
        System.out.printf("Order total:          $%.2f%n", smallOrder);
        System.out.printf("Buggy price (int):    $%.2f  <-- customer overcharged!%n", buggyResult);
        System.out.printf("Correct price (double):$%.2f  <-- proper 10%% discount%n", fixedResult);
    }
}

/**
 * This file simulates a real scenario: a UserRepository that a reviewer
 * would flag for a performance problem. The comments below demonstrate
 * the difference between a BAD review comment and a GOOD review comment
 * on the exact same issue.
 */
import java.util.List;
import java.util.ArrayList;

public class ReviewCommentExamples {

    // Simulated in-memory 'database' of users
    static List<String> allUsersInDatabase = new ArrayList<>();

    static {
        // Pre-populate with 10,000 fake users to illustrate the scale problem
        for (int i = 0; i < 10_000; i++) {
            allUsersInDatabase.add("user_" + i + "@example.com");
        }
    }

    /**
     * ORIGINAL CODE (the author's PR):
     * Finds active users whose email matches a search term.
     *
     * BAD review comment: "This is inefficient."
     *   — Why it fails: gives no context, no suggestion, sounds like a criticism of the person
     *
     * GOOD review comment:
     *   [blocking] This loads all 10,000+ users into memory on every search request
     *   (observation). At our current growth rate we'll hit OOM errors within a month
     *   (impact). Could we push the LIKE filter into the SQL query so only matching
     *   rows are fetched? Something like: SELECT * FROM users WHERE email LIKE ?
     *   (suggestion). Happy to pair on this if the query gets complex.
     */
    public static List<String> findUsersByEmailTerm_Original(String searchTerm) {

        // Loads ALL users then filters in Java — bad at scale
        List<String> allUsers = allUsersInDatabase; // imagine this is a DB call
        List<String> matchingUsers = new ArrayList<>();

        for (String email : allUsers) {
            if (email.contains(searchTerm)) { // filtering in application memory
                matchingUsers.add(email);
            }
        }
        return matchingUsers;
    }

    /**
     * IMPROVED CODE (what the reviewer's suggestion leads to):
     * In a real app the filter moves to SQL. Here we simulate that by
     * treating it as a pre-filtered list arriving from the DB layer.
     */
    public static List<String> findUsersByEmailTerm_Improved(String searchTerm) {

        // Simulates: SELECT email FROM users WHERE email LIKE '%searchTerm%'
        // Only matching rows cross the network — memory stays flat
        List<String> dbFilteredResults = simulateDatabaseQuery(searchTerm);

        return dbFilteredResults; // no redundant in-memory loop needed
    }

    /** Simulates a parameterized DB query that filters server-side */
    private static List<String> simulateDatabaseQuery(String searchTerm) {
        List<String> results = new ArrayList<>();
        for (String email : allUsersInDatabase) {
            if (email.contains(searchTerm)) {
                results.add(email);
            }
        }
        return results;
    }

    public static void main(String[] args) {

        String testSearch = "user_42";

        long startOriginal = System.nanoTime();
        List<String> originalResult = findUsersByEmailTerm_Original(testSearch);
        long durationOriginal = System.nanoTime() - startOriginal;

        long startImproved = System.nanoTime();
        List<String> improvedResult = findUsersByEmailTerm_Improved(testSearch);
        long durationImproved = System.nanoTime() - startImproved;

        System.out.println("Results match: " + originalResult.equals(improvedResult));
        System.out.printf("Original approach: %,d ns (all users loaded into memory)%n", durationOriginal);
        System.out.printf("Improved approach: %,d ns (only matched rows)%n", durationImproved);
        System.out.println("\nComment style matters as much as the finding itself.");
    }
}

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;

/**
 * This file demonstrates two security issues a reviewer MUST catch:
 * 1. SQL injection via string concatenation
 * 2. A sensitive value being logged
 *
 * Both patterns appear in real PRs. Both are easy to fix once spotted.
 */
public class SecurityReviewChecklist {

    private static final String DB_URL = "jdbc:h2:mem:testdb";

    /**
     * VULNERABLE: builds SQL by concatenating user-supplied input.
     *
     * REVIEWER COMMENT: [blocking][security]
     * Concatenating `username` directly into the query string allows SQL injection.
     * A username of:  ' OR '1'='1
     * ...turns this into: SELECT * FROM users WHERE username = '' OR '1'='1'
     * which returns every user in the table.
     * Fix: use a PreparedStatement with a parameterized query (see below).
     */
    public static ResultSet findUser_Vulnerable(Connection conn, String username)
            throws SQLException {

        // NEVER do this — user controls the query structure
        String rawQuery = "SELECT * FROM users WHERE username = '" + username + "'";

        // Also: logging the raw username leaks PII to your log aggregator
        System.out.println("[INSECURE LOG] Executing query: " + rawQuery);

        return conn.createStatement().executeQuery(rawQuery);
    }

    /**
     * SAFE: uses a PreparedStatement — the DB treats the parameter as
     * data, never as executable SQL, regardless of what it contains.
     */
    public static ResultSet findUser_Safe(Connection conn, String username)
            throws SQLException {

        // ? is a bind parameter — the DB driver handles escaping
        String safeQuery = "SELECT id, username FROM users WHERE username = ?";

        PreparedStatement statement = conn.prepareStatement(safeQuery);
        statement.setString(1, username); // bind user input as data, not SQL

        // Log intent without exposing the actual input value
        System.out.println("[SAFE LOG] Querying users table with parameterized lookup");

        return statement.executeQuery();
    }

    public static void main(String[] args) throws Exception {

        // Set up a lightweight in-memory DB to demonstrate the concepts
        Connection connection = DriverManager.getConnection(DB_URL, "sa", "");
        connection.createStatement().execute(
            "CREATE TABLE users (id INT PRIMARY KEY, username VARCHAR(100))"
        );
        connection.createStatement().execute(
            "INSERT INTO users VALUES (1, 'alice'), (2, 'bob')"
        );

        String maliciousInput = "' OR '1'='1";

        System.out.println("=== Security Review Demo ===");
        System.out.println("\n-- Vulnerable approach (do NOT approve) --");
        try {
            ResultSet rs = findUser_Vulnerable(connection, maliciousInput);
            int rowCount = 0;
            while (rs.next()) rowCount++;
            System.out.println("Rows returned by injection attack: " + rowCount + " (expected: 0)");
        } catch (SQLException e) {
            System.out.println("Query error: " + e.getMessage());
        }

        System.out.println("\n-- Safe approach (parameterized) --");
        ResultSet safeRs = findUser_Safe(connection, maliciousInput);
        int safeRowCount = 0;
        while (safeRs.next()) safeRowCount++;
        System.out.println("Rows returned with parameterized query: " + safeRowCount + " (correct)");

        connection.close();
    }
}

/**
 * This is not runnable code — it's a PR description template formatted as
 * a Java comment block to fit the article format.
 *
 * A good PR description is half the battle. Reviewers approve faster
 * when they understand intent before they read a single line of code.
 *
 * ─────────────────────────────────────────────
 * WHAT MAKES A GREAT PR DESCRIPTION (Template)
 * ─────────────────────────────────────────────
 *
 * ## What does this PR do?
 * Adds a loyalty discount calculation to the order checkout flow.
 * Customers who have been registered for 365+ days receive 10% off.
 *
 * ## Why are we doing this?
 * Addresses customer retention feedback from Q3 survey (Ticket: RET-441).
 * Loyalty customers were churning because we had no visible benefit to staying.
 *
 * ## How does it work?
 * - New `OrderDiscountService` calculates discount based on account age
 * - Discount applied in `CheckoutController` before payment is initiated
 * - Rate and threshold are config-driven (see application.yml change)
 *
 * ## What should the reviewer focus on?
 * - The discount calculation in `calculateLoyaltyPrice()` — I want a sanity
 *   check that the rounding behaviour is correct for small order values
 * - The new index on `users.created_at` in the migration — is this the
 *   right column to index or should it be a composite index?
 *
 * ## Testing
 * - Unit tests cover: no discount below threshold, exact threshold day,
 *   above threshold, zero-value order, max double order value
 * - Manual test: created a test account dated 2 years ago, confirmed
 *   10% applied correctly at checkout
 *
 * ## What's NOT in this PR?
 * Email notification to loyalty customers is out of scope — tracked in RET-445.
 * ─────────────────────────────────────────────
 */
public class PullRequestDescriptionTemplate {

    public static void main(String[] args) {
        System.out.println("A good PR description answers 5 questions:");
        System.out.println("1. WHAT:  What does this change?");
        System.out.println("2. WHY:   What business/technical reason?");
        System.out.println("3. HOW:   High-level approach (not line-by-line)");
        System.out.println("4. FOCUS: What do you specifically want eyes on?");
        System.out.println("5. SCOPE: What is deliberately NOT included?");
        System.out.println();
        System.out.println("Reviewers with good context approve 60%+ faster.");
        System.out.println("Context reduces nit-picks and increases real bug finds.");
    }
}