Effective Code Reviews: What Senior Engineers Should Focus On

Code review is one of the highest-leverage practices in software engineering. Done well, it catches defects before they reach production, distributes knowledge, and improves the overall quality of the codebase. Done poorly, it’s a bureaucratic checkpoint that blocks delivery while catching only superficial issues.

The difference is usually what the reviewer focuses on.

What Automated Tools Should Catch

The first principle of effective code review: don’t spend human review time on things that tools can detect.

Formatting, indentation, line length, import ordering, unused variables, obvious null dereferences, common security anti-patterns — these should be caught by:

Linters (Checkstyle, PMD, SpotBugs)
Formatters (Spotless, google-java-format)
Static analysis (SonarQube, Error Prone)
Security scanners (OWASP Dependency Check, Semgrep)

Running these automatically in CI means they’re enforced consistently without reviewer time. When a reviewer comments “this method is too long” or “use camelCase here,” they’re doing work that should be automated.

Configure the tools, agree on the standards, enforce them in CI, and then your reviewers can spend time on things that matter.

Correctness: The Primary Concern

The most important question in any code review: does this code do what it claims to do?

This is harder than it looks. Confirming correctness requires understanding:

What the code is supposed to do (from the ticket, design doc, or tests)
What the code actually does (from reading it carefully)
What the tests verify (and what they don’t)

Look for:

Off-by-one errors in loops, ranges, pagination
Incorrect comparisons (== vs .equals() in Java, reference vs value equality)
Missing null checks for values that can realistically be null
Race conditions in code that runs concurrently
Integer overflow in arithmetic on potentially large values
Incorrect assumptions about ordering, uniqueness, or completeness

// A correctness issue that's easy to miss
public Page<Order> findOrdersBefore(Instant cutoff, Pageable pageable) {
    return orderRepository.findByCreatedAtBefore(cutoff, pageable);
}

Is createdAtBefore inclusive or exclusive? If the caller expects inclusive and the repository query is exclusive (or vice versa), one request’s worth of orders is silently dropped or duplicated. Test it.

API Design: The Contract

When code exposes a method, class, or interface that other code will depend on, the API design deserves scrutiny beyond “does it work.”

Is the method signature honest? A method named getUser that might return null should either be Optional<User> findUser(...) or User getUser(...) throws UserNotFoundException. The name and return type should accurately represent behavior.

Can it be misused? A method that has two boolean parameters is likely to be called incorrectly:

// Easy to get parameters wrong — what does (true, false) mean?
processOrder(orderId, true, false);

// Better: use an explicit parameter object or builder
processOrder(orderId, ProcessingOptions.builder()
    .sendConfirmation(true)
    .chargeImmediately(false)
    .build());

Is it too broad? A method that accepts Object or generic types when it should accept a specific type loses type safety. A method that returns a mutable collection when it should return an immutable view leaks internal state.

Does it handle edge cases explicitly? Empty collections, null inputs, invalid ranges — what happens? Is it documented? Is it tested?

Concurrency: Subtle and Dangerous

Concurrency issues are some of the hardest bugs to reproduce and debug. Code review is an excellent place to catch them.

Things to check:

Shared mutable state: any field that’s accessed by multiple threads without synchronization is a data race.

@Service
public class CounterService {
    private int count = 0;  // NOT thread-safe!
    
    public void increment() { count++; }  // Read-modify-write is not atomic
    public int getCount() { return count; }
}

Use AtomicInteger, volatile, or synchronization appropriately.

Lock ordering: if code acquires multiple locks, is the order consistent? Inconsistent lock ordering causes deadlocks.

Check-then-act: the classic race condition pattern:

// Race condition — another thread may change the state between check and act
if (order.getStatus() == PENDING) {
    order.setStatus(PROCESSING);  // Not atomic with the check above
}

Use atomic operations, optimistic locking, or explicit synchronization.

ThreadLocal in virtual threads: with Java 21 virtual threads, ThreadLocal semantics change in subtle ways. Code that assumed thread pool reuse may not behave correctly.

Failure Modes: How Does It Break?

For every significant operation, ask: what happens when this fails?

What happens when the database is unavailable?
What happens when the HTTP client times out?
What happens when the message queue is full?
What happens if this throws midway through a multi-step operation?

// What happens if the event publish fails after the DB save?
public void placeOrder(Order order) {
    orderRepository.save(order);        // Succeeds
    eventPublisher.publish(orderPlaced); // Fails — order saved but event not published
}

This is the outbox pattern problem. The reviewer should catch this.

Look for missing finally blocks, missing error handling in async code, missing rollback logic in multi-step operations, and swallowed exceptions.

Security: What’s the Threat Model?

Every change that touches input handling, authentication, authorization, or data exposure deserves security review.

Authorization checks: does every operation that reads or modifies sensitive data verify that the caller has permission?

@GetMapping("/orders/{id}")
public OrderResponse getOrder(@PathVariable String id) {
    return orderService.findOrder(id)  // Who is allowed to see this order?
        .map(orderMapper::toResponse)
        .orElseThrow(OrderNotFoundException::new);
}

Is there a check that the authenticated user owns this order, or has permission to view it? If not, any authenticated user can view any order.

Input validation: is user-supplied input validated before use? Are database queries parameterized? Is HTML escaped before rendering?

Data exposure: is the response returning more data than the caller needs? Is sensitive data (passwords, payment details, PII) excluded from logs and responses?

Observability: Can You Debug This in Production?

Code that works correctly but produces no observable signal when it doesn’t is operationally expensive.

Check for:

Missing error logging: errors that are caught but not logged make incidents invisible
Missing metrics: significant operations without instrumentation create monitoring blind spots
Missing correlation IDs: log statements without trace context make distributed debugging impossible
Uninformative log messages: “error occurred” is not a log message

// Hard to debug in production
catch (Exception e) {
    log.error("Error");  // Which error? For what order? From which user?
    return ResponseEntity.status(500).build();
}

// Debuggable
catch (PaymentGatewayException e) {
    log.error("Payment processing failed for order {} (customer {}): {}",
        order.id(), order.customerId(), e.getMessage(), e);
    metrics.counter("payment.gateway.error", "error_type", e.getType()).increment();
    return ResponseEntity.status(502).body(ErrorResponse.paymentFailed());
}

Performance: When It Matters

Performance review is context-dependent. Not every code path requires performance scrutiny. Focus on:

Paths called in hot loops (per-request, per-message)
Database queries (N+1 patterns, missing pagination)
Memory allocation in tight loops
Synchronous operations that could be async

// N+1 query — loads each user's orders separately
List<User> users = userRepository.findAll();
for (User user : users) {
    user.setOrderCount(orderRepository.countByCustomerId(user.getId())); // N queries
}

// Fix: JOIN or a single bulk query
Map<UserId, Long> orderCounts = orderRepository.countByCustomerIds(userIds);

Don’t optimize preemptively. Profile first. But obvious N+1 patterns visible in code review are worth flagging.

What Makes a Good Review Comment

Good review comments:

Explain why: “this is a data race because…” not just “this is wrong”
Are specific: “line 42 is missing an authorization check” not “security issue”
Suggest solutions: “consider using @PreAuthorize or checking ownership here”
Distinguish blocking from advisory: mark what must be fixed vs what’s a suggestion

Bad review comments:

“nit: rename this to X” (automation should handle naming conventions)
“I would do this differently” (without explaining the trade-off)
“this is wrong” (without explanation)
Long discussions of style in a patch that has correctness issues

The goal of a code review is to improve the code and the engineer who wrote it, not to demonstrate the reviewer’s knowledge or enforce personal preferences.