看了 1000 多个 PR 之后，这七个错误每次都出现

最后

如果你在审阅自己的PR时，也常常撞见上面这些问题，先别急着懊恼。这恰恰说明，你的关注点已经开始从“代码能不能跑通”，转向了“代码能不能长期、稳定地活下去”。而真正扎实的工程能力，往往就是从这一步开始生根发芽的。

免费影视、动漫、音乐、游戏、小说资源长期稳定更新！ 👉 点此立即查看 👈

做了三年技术负责人，审过的代码评审少说也有几百个。大概在审到第400个左右的时候，一个有点可怕的发现浮出水面：

大家掉进去的坑，翻来覆去总是那么几种。

不同的工程师，不同的公司，不同的代码库。

可最终，那些让人头疼的问题，其“配方”却惊人地相似。

它们通常不是语法错误——那种问题编译器早就帮你揪出来了。也不是一眼就能看穿的逻辑漏洞——那种往往在测试阶段就能被发现。

真正麻烦的，是那种今天看着一切正常，三个月后却能在生产环境里悄无声息地给你致命一击的东西。

下面这七类问题，几乎是我在PR里最常遇到的“回头客”。

1. 代码能跑，但根本没法读

最折磨人的PR，不是代码错得离谱。而是它技术上完全正确，却要你花上二十分钟，才能勉强弄明白它到底想干什么。

// 我最常看到的写法
boolean r = u != null && u.getS() != null && u.getS().equals("ACTIVE")
    && !u.getF() && u.getC() > System.currentTimeMillis();

// 它本来应该更像这样
boolean isUserEligible = user != null
    && user.getStatus() != null
    && user.getStatus().equals("ACTIVE")
    && !user.isFrozen()
    && user.getContractExpiresAt() > System.currentTimeMillis();

这两段代码，逻辑上干的是同一件事。

但区别在于：有一段，是你凌晨三点被线上报警叫醒时，还能在睡眼惺忪中看懂的；另一段，是你盯着屏幕看半天，越看越烦躁、越看越不确定的。

在这种PR下面，我最常留的评论就是：

“四个月后的你，还看得懂吗？值班同事凌晨三点打开这段代码时，还能立刻理解吗？”

如果这两个问题的答案都是“否”，那就别犹豫，重写。

代码从来不是写给编译器看的。代码迟早都是写给人看的。而且通常是在最狼狈、最着急的时候看。

2. 错误处理把真正的问题藏起来了

这种代码模式，真的见过无数次，而且它非常容易一路绿灯混进生产环境：

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

乍一看，它甚至有点“工整”：异常被捕获了，日志也打了，返回值也处理了。

似乎没什么问题。

可它在生产环境里通常会演变成什么样？

结账流程开始随机失败。前端只知道失败了。后端日志里永远只有孤零零的一句：“Payment failed”。

然后呢？

没人知道到底是怎么失败的。是网络超时？数据库断连？空指针？授权失败？还是第三方接口限流了？

于是，三个小时过去了，团队还在日志的海洋里打捞线索。最后才发现：真正的异常细节，早就被这段“看起来很稳妥”的处理逻辑给吞掉了。

更合理的写法，通常应该更像这样：

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

不同类型的异常，往往意味着完全不同的处理策略。有些该触发重试，有些该直接标记失败，有些该触发告警，有些该启用降级方案。

把所有异常一把抓住，然后统一吞掉，本质上就是在埋定时冲击波。它不会立刻爆炸，但它迟早会在你最不希望它爆炸的时候，给你来个措手不及。

3. 对线程安全完全没开过脑内会议

这个模式，我至少见过几十次：

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

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

在开发环境里，它通常跑得非常正常。本地一测，丝滑流畅。单线程调试，更是毫无问题。

可一到生产环境，只要并发请求一上来，各种你不想见到的事情就可能发生：缓存被并发写坏，同一条数据被重复加载多次，冷不丁抛出ConcurrentModificationException，行为变得时好时坏，极难复现。

这种问题最讨厌的地方在于：它不是那种在单元测试里轻轻松松就能暴露的bug。

它通常会挑什么时间出现？上午十一点，业务高峰，流量突然上来，然后报警开始响个不停。

解决方案可能很简单。有时候换成ConcurrentHashMap就够了；有时候需要加锁；有时候则需要重新设计缓存更新策略。

但在修复之前，你得先意识到这件事本身是个问题。而很多评审者，恰恰会漏掉这一步。

所以现在只要在PR里看到共享的可变状态，脑子里第一反应就是一句话：

“如果两个线程同时打到这里，会发生什么？”

只要这个问题答不清楚，这段代码就不能算安全。

4. 循环里查数据库：经典到不能再经典，但还是有人每周都写

这就是臭名昭著的N+1查询问题。它太有名了，以至于几乎人人都知道它。可神奇的是，它依然频繁出现在每隔一两个涉及数据访问的PR里。

// 很多人就这样提交上来了
List orders = orderRepository.findAll();
for (Order order : orders) {
    User user = userRepository.findById(order.getUserId()); // 循环里查 DB
    emailService.send(user.getEmail(), buildConfirmation(order));
}

如果只有100个订单，这就是101次数据库查询。如果有10000个订单，那就是10001次查询。

在开发环境里为什么不容易暴露？因为本地数据库的表里可能就躺着20行测试数据。跑起来当然没感觉。可一旦对上真实生产环境的数据量，数据库的CPU很容易直接被打满。

更合理的做法，通常是先批量取出数据，再在内存里进行映射：

List orders = orderRepository.findAll();
Set userIds = orders.stream()
    .map(Order::getUserId)
    .collect(Collectors.toSet());
Map 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));
}

这样下来，不管订单有多少条，数据库查询都只有两次。

老实说，这类问题我一般都会直接拒绝合并。没有例外。

这不是性能洁癖，也不是吹毛求疵。

而是因为，我不想凌晨三点被电话叫醒，然后得知某个批处理任务把数据库打趴了，而原因只是因为有人把查询写进了循环里。

5. 把配置当常量写死，是很多故障的起点

这种写法你肯定见过：

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";

这些值在一开始通常都“没问题”。问题在于，它们只会在“现在”没问题。

可生产环境也许需要更长的超时时间，因为网络延迟更高；测试环境也许还只能走v1接口，因为v2根本还没部署；压测环境可能希望重试次数直接设为0，这样失败能更快暴露出来。

可你把值写死之后，哪怕只是想改个超时时间，也得重新走一遍发版流程。

更好的方式，通常应该是配置化：

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

代码行为没变。但现在，不同环境终于可以有不同参数，而不必为了改个配置值就去修改代码、提交PR、重新部署一遍。

我在看这类PR时，脑子里也有一句固定问题：

“这个值，会不会在所有环境里、永远都一样？”

只要这个问题有一点点不确定，它就不该被写死在代码里。

6. 只写“成功路径”，从来不想“失败了怎么办”

绝大多数代码，都是照着“快乐路径”写出来的。而真正的生产事故，几乎都躲在“不快乐路径”里。

比如这种：

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

看到这种代码，我在PR里通常会连着问好几个问题：如果getUser返回的是null怎么办？如果外部API挂了怎么办？如果外部API调用成功了，但sa ve失败了怎么办？如果同一个用户被并发触发两次同步，会发生什么？

这些不是“边界情况”。它们就是生产环境里最常发生的日常。

很多人写代码的时候，会默认整个世界都配合自己：服务总在线，数据总存在，调用总成功，存储总稳定，执行总按顺序。

可现实从来不是这样。

真正能优雅处理失败的代码，往往不酷。它甚至看上去更长、更啰嗦、更“没那么漂亮”。

但它和线上事故之间，往往就只差这些“麻烦的显式处理”。

很多时候，系统的稳定性不是来自聪明。稳定性来自你愿不愿意提前把各种坏情况都想清楚。

7. 一个 PR 想顺手把全世界都改了

最后一个问题，不完全是代码问题，更像一种团队协作中的习惯病。

你一定见过这种PR：改了47个文件，新增800行，删除600行。然后描述里轻描淡写地写着一句：

“重构了用户服务，顺便加了新的支付流程，修了上周那个bug，还升级了一下依赖。”

这种PR，不是“难review”。是根本没法被认真review。

你可以看，也可以点approve。但你没法真正验证它。一定会有东西被漏掉，而且几乎每次都会漏。

真正容易review好的PR，通常是什么样？300行以内更理想，目标单一，改动边界明确，出问题后容易回滚。

只要我看到那种“巨无霸PR”，我通常就会回一句：

“能不能拆小一点？不是因为我不信任你，而是因为没有人能真正认真审完800行改动。”

对方常见的反驳是：“这些改动都是相关的。”

有时候，这话是真的。但大多数时候，并不是。

更多时候，真实情况只是：重构、修bug、加新功能，恰好被同一个人、在同一段时间里一起做完了而已。

“同时做完”不等于“必须一起提”。