Code reviews aren't about catching syntax errors—the compiler does that for us. They're about mentorship, consistency, and preventing the "technical debt" that keeps you awake at 3:00 AM on a Tuesday. When I review a junior's PR, or when a senior tears into mine, we aren't looking for "correctness" in the mathematical sense; we're looking for maintainability.

To show you what I mean, let's build a small DiscountService. This service will decide how much of a discount to give a customer based on their membership tier. I'll write this as if I'm rushing to meet a deadline, and then I'll "review" my own code to show you the habits I actually use in production.

Writing the first draft of the DiscountService

Here is my initial implementation. It works. If you run it with a "GOLD" member, they get 20% off. Simple, right?

public class DiscountService {
    public double calculateDiscount(User user, double totalAmount) {
        if (user.getMembershipLevel().equals("GOLD")) {
            return totalAmount * 0.20;
        } else if (user.getMembershipLevel().equals("SILVER")) {
            return totalAmount * 0.10;
        }
        return 0.0;
    }
}

Now, if I were reviewing this in a GitHub PR, my first thought wouldn't be "this works," but "how easily can this break?"

Catching the obvious crash

Wait—I just spotted a classic mistake. Look at user.getMembershipLevel().equals("GOLD"). I've fallen into the "NullPointerException Trap." If getMembershipLevel() returns null, the whole application crashes. I've seen this happen in production more times than I care to admit.

A seasoned Java developer avoids this by putting the constant first. It's a small habit, but it's a lifesaver. Let's fix that immediately.

// Much safer: "GOLD" is never null, so this won't throw an NPE
if ("GOLD".equals(user.getMembershipLevel())) { 
    return totalAmount * 0.20;
}

Eliminating "Magic Strings" and Rigid Logic

Even with the null fix, this code is brittle. Those strings—"GOLD" and "SILVER"—are what we call "Magic Strings." If someone typos "GOLS" elsewhere in the app, the discount fails silently. In a real review, I'd insist on moving these to an Enum. It makes the code self-documenting and type-safe.

I'm also noticing that the logic is just a series of if-else blocks. If we add ten more membership levels, this method becomes a nightmare to read. I'll refactor this to use a Map or a switch expression (available in modern Java) to keep the mapping clean.

public enum MembershipLevel {
    GOLD, SILVER, BRONZE, NONE
}

public class DiscountService {
    private static final Map<MembershipLevel, Double> DISCOUNTS = Map.of(
        MembershipLevel.GOLD, 0.20,
        MembershipLevel.SILVER, 0.10,
        MembershipLevel.BRONZE, 0.05
    );

    public double calculateDiscount(User user, double totalAmount) {
        MembershipLevel level = user.getMembershipLevel();
        double rate = DISCOUNTS.getOrDefault(level, 0.0);
        return totalAmount * rate;
    }
}

Reviewing for API Intent

The final habit is questioning the API design. Is calculateDiscount returning the amount *to be subtracted* or the *final price*? The name is slightly ambiguous. In a professional project, I'd ask the author to be explicit. Let's rename it to calculateDiscountAmount to remove any doubt.

I'd also check for final modifiers. In Java, if a variable shouldn't change, mark it final. It tells the next developer, "Don't touch this value," and helps the JVM optimize the code. Look at the final version below:

public class DiscountService {
    private static final Map<MembershipLevel, Double> DISCOUNTS = Map.of(
        MembershipLevel.GOLD, 0.20,
        MembershipLevel.SILVER, 0.10,
        MembershipLevel.BRONZE, 0.05
    );

    public double calculateDiscountAmount(final User user, final double totalAmount) {
        final MembershipLevel level = user.getMembershipLevel();
        final double rate = DISCOUNTS.getOrDefault(level, 0.0);
        return totalAmount * rate;
    }
}

The difference between the first version and the last isn't just "cleaner code"—it's the difference between a system that crashes on edge cases and one that is robust, scalable, and easy for a teammate to understand without needing a meeting to explain it.

📋 Practical Task

Refactoring the Legacy UserValidator

You have been assigned to review a piece of legacy code in the UserValidator class. The current implementation is functional but violates several of the code review habits discussed in this lesson. It is prone to NullPointerException, uses magic strings, and lacks finality.

public class UserValidator {
    public boolean isValid(User user) {
        if (user.getRole().equals("ADMIN")) {
            return user.getPermissions() != null;
        }
        if (user.getRole().equals("USER")) {
            return user.getEmail() != null && user.getEmail().contains("@");
        }
        return false;
    }
}

Your Task: Refactor this class to implement professional Java habits. You must:

• Replace the "Magic Strings" ("ADMIN", "USER") with a Role Enum.
• Fix the potential NullPointerException when checking the role.
• Apply the final keyword to parameters and local variables that do not change.
• Improve the method's robustness (e.g., handling a null user object at the start of the method).