7 Recurring Mistakes I See in Every PR After Reviewing Over 1,000

As a tech lead who has reviewed around 400 PRs, I started noticing a frightening pattern: the same handful of mistakes keep showing up across different engineers, companies, and codebases. These errors are not syntax bugs or obvious logic flaws; they are the subtle issues that look fine today but silently break production weeks later.

1. Code Runs but Is Impossible to Read

The most painful PRs are not those with glaring bugs, but those whose logic is technically correct yet requires twenty minutes to understand. For example:

// Common style I see
boolean r = u != null && u.getS() != null && u.getS().equals("ACTIVE") && !u.getF() && u.getC() > System.currentTimeMillis();

A clearer version would be:

boolean isUserEligible = user != null && user.getStatus() != null && user.getStatus().equals("ACTIVE") && !user.isFrozen() && user.getContractExpiresAt() > System.currentTimeMillis();

The latter can be understood quickly even during a 3 am incident, while the former forces the reviewer to stare at the screen for a long time. I always leave a comment like “Will you still understand this in four months? Can the on‑call engineer read it at 3 am?” and ask for a rewrite if the answer is no.

2. Error Handling Hides the Real Problem

A typical pattern that easily slips into production is catching a generic Exception and logging only a vague message:

try {
    processPayment(order);
} catch (Exception e) {
    log.error("Payment failed");
    return false;
}

This looks tidy, but in production the checkout randomly fails and the logs only contain “Payment failed”. No one knows whether it was a network timeout, a database disconnect, a null pointer, an auth error, or a third‑party rate‑limit. After hours of digging, the real exception details are discovered to have been swallowed.

A better approach distinguishes exception types and preserves context:

try {
    processPayment(order);
} catch (PaymentNetworkException e) {
    log.error("Order {} payment network timeout: {}", order.getId(), e.getMessage());
    throw new RetryableException("Payment network unavailable", e);
} catch (PaymentAuthException e) {
    log.error("Order {} payment authorization failed: {}", order.getId(), e.getMessage());
    return PaymentResult.AUTHORIZATION_FAILED;
}

Different exceptions imply different handling strategies—retry, fail fast, alert, or degrade.

3. Ignoring Thread‑Safety Leads to Concurrency Bugs

A classic example I see dozens of times:

public class UserCache {
    private Map<String, User> cache = new HashMap<>();

    public User getUser(String id) {
        if (!cache.containsKey(id)) {
            cache.put(id, loadFromDB(id));
        }
        return cache.get(id);
    }
}

In a single‑threaded test it works flawlessly, but under load the cache can be corrupted, duplicate loads occur, ConcurrentModificationException may be thrown, and behavior becomes flaky and hard to reproduce. The fix is often as simple as switching to ConcurrentHashMap, adding proper locking, or redesigning the update strategy.

4. N+1 Queries Inside Loops

Many PRs still contain the classic N+1 problem:

List<Order> orders = orderRepository.findAll();
for (Order order : orders) {
    User user = userRepository.findById(order.getUserId());
    emailService.send(user.getEmail(), buildConfirmation(order));
}

With 10 000 orders this triggers 10 001 database calls. A better solution batches the data first:

List<Order> orders = orderRepository.findAll();
Set<Long> userIds = orders.stream().map(Order::getUserId).collect(Collectors.toSet());
Map<Long, User> users = userRepository.findAllById(userIds)
    .stream().collect(Collectors.toMap(User::getId, u -> u));
for (Order order : orders) {
    User user = users.get(order.getUserId());
    emailService.send(user.getEmail(), buildConfirmation(order));
}

This reduces the database round‑trips to two, regardless of order count. I usually reject such PRs outright.

5. Hard‑Coding Configuration Values

Hard‑coded constants like timeouts, retry limits, or API URLs look fine initially but become liabilities when environments differ:

private static final int TIMEOUT = 5000;
private static final int MAX_RETRIES = 3;
private static final String API_URL = "https://api.payment.com/v2/charge";

Production may need a longer timeout, test environments might still use v1 of an API, and load‑testing may want zero retries. By externalising these values:

@Value("${payment.timeout:5000}")
private int paymentTimeout;

@Value("${payment.maxRetries:3}")
private int maxRetries;

@Value("${payment.apiUrl}")
private String apiUrl;

the same code runs unchanged across environments, and the question “Is this value the same in every environment?” guides the review.

6. Ignoring the Unhappy Path

Most code follows the happy path and neglects failure handling. Consider:

public void syncUserData(String userId) {
    User user = userService.getUser(userId);
    ExternalProfile profile = externalApi.getProfile(user.getExternalId());
    userRepository.save(user.withProfile(profile));
}

Reviewers should ask: What if getUser returns null? What if the external API is down? What if save fails? What if two syncs run concurrently? These “edge cases” are the daily cause of production incidents. Robust code often looks longer and less “pretty”, but it prevents silent failures.

7. “Mega” PRs That Try to Change the Whole World

Sometimes a PR touches dozens of files, adds hundreds of lines, and deletes many others, with a description like “Refactored user service, added new payment flow, fixed last week’s bug, and upgraded dependencies.” Such PRs are impossible to review thoroughly; they hide defects. A reviewable PR is typically under 300 lines, has a single purpose, clear change boundaries, and can be rolled back easily. When I see a massive PR I ask, “Can you split it? Not because I distrust you, but because no one can seriously review 800 lines.” The common rebuttal “All these changes are related” is often false—refactoring, bug fixes, and new features should be submitted separately.

Conclusion

If you recognize these patterns in your own PRs, it’s a sign you’re moving from “does the code compile?” to “can the code survive long‑term in production?” Mature engineering capability grows from addressing these recurring pitfalls.