跳到主要内容

审查实战案例

理论学习固然重要，但代码审查是一门实践性很强的技能。本章通过真实的审查案例，展示如何将理论应用于实践。

案例 1：空值处理缺陷​

场景描述​

开发者提交了一个用户信息查询功能，审查者发现潜在的空指针异常风险。

提交的代码​

public class UserService {
    
    @Autowired
    private UserRepository userRepository;
    
    public String getUserCity(Long userId) {
        User user = userRepository.findById(userId);
        return user.getAddress().getCity();
    }
}

审查对话​

审查者：

🔴 阻塞：这段代码存在空指针异常风险。

问题分析：

  1. findById() 可能返回 null（用户不存在）
  2. user.getAddress() 可能返回 null（用户未设置地址）

两种情况都会导致 NullPointerException，在生产环境中可能造成服务不可用。

建议修改：

public String getUserCity(Long userId) {
    User user = userRepository.findById(userId);
    if (user == null) {
        throw new UserNotFoundException("用户不存在: " + userId);
    }
    
    Address address = user.getAddress();
    if (address == null) {
        return "未设置地址";  // 或者抛出业务异常
    }
    
    return address.getCity();
}

开发者：

感谢指出这个问题！已修复。

我采用了你的建议，并且：

  1. 添加了 UserNotFoundException 异常
  2. 对于没有地址的用户，返回默认值 "未设置地址"
  3. 添加了单元测试覆盖这些边界情况

经验总结​

  • 链式调用 a.b.c() 是空指针的高发区域
  • 对外部输入（如 userId）和数据库查询结果都要进行空值检查
  • 边界情况的处理方式（抛异常、返回默认值、返回 Optional）应该与团队规范一致

案例 2：SQL 注入漏洞​

场景描述​

开发者在实现搜索功能时，直接拼接用户输入到 SQL 语句中。

提交的代码​

public List<User> searchUsers(String keyword) {
    String sql = "SELECT * FROM users WHERE name LIKE '%" + keyword + "%'";
    return jdbcTemplate.query(sql, new UserRowMapper());
}

审查对话​

审查者：

🔴 阻塞：严重的 SQL 注入漏洞！

攻击者可以通过精心构造的 keyword 值来执行任意 SQL：

keyword = "'; DROP TABLE users; --"

这会生成以下 SQL：

SELECT * FROM users WHERE name LIKE '%'; DROP TABLE users; --%'

必须使用参数化查询：

public List<User> searchUsers(String keyword) {
    String sql = "SELECT * FROM users WHERE name LIKE ?";
    String searchPattern = "%" + keyword + "%";
    return jdbcTemplate.query(sql, new UserRowMapper(), searchPattern);
}

开发者：

已修复！感谢指出这个安全问题。

我改用了 NamedParameterJdbcTemplate，代码更清晰：

public List<User> searchUsers(String keyword) {
    String sql = "SELECT * FROM users WHERE name LIKE :keyword";
    Map<String, Object> params = Map.of("keyword", "%" + keyword + "%");
    return namedJdbcTemplate.query(sql, params, new UserRowMapper());
}

经验总结​

  • 永远不要信任用户输入，所有外部输入都是潜在的安全威胁
  • 使用参数化查询或 ORM 框架来防止 SQL 注入
  • 安全漏洞必须在合并前修复，不能妥协

案例 3：性能问题​

场景描述​

开发者在循环中执行数据库查询，可能导致 N+1 问题。

提交的代码​

public List<OrderDTO> getOrdersWithDetails(List<Long> orderIds) {
    List<OrderDTO> result = new ArrayList<>();
    
    for (Long orderId : orderIds) {
        Order order = orderRepository.findById(orderId);
        List<OrderItem> items = orderItemRepository.findByOrderId(orderId);
        User user = userRepository.findById(order.getUserId());
        
        OrderDTO dto = new OrderDTO();
        dto.setOrder(order);
        dto.setItems(items);
        dto.setUserName(user.getName());
        result.add(dto);
    }
    
    return result;
}

审查对话​

审查者：

💡 建议：这段代码存在 N+1 查询问题。

如果 orderIds 有 100 个元素，这段代码会执行：

  • 100 次 order 查询
  • 100 次 orderItem 查询
  • 100 次 user 查询
  • 共计 300 次数据库查询

在生产环境中可能严重影响性能。

建议改为批量查询：

public List<OrderDTO> getOrdersWithDetails(List<Long> orderIds) {
    // 一次查询获取所有订单
    List<Order> orders = orderRepository.findAllById(orderIds);
    
    // 一次查询获取所有订单项
    List<OrderItem> allItems = orderItemRepository.findByOrderIdIn(orderIds);
    Map<Long, List<OrderItem>> itemsByOrderId = allItems.stream()
        .collect(Collectors.groupingBy(OrderItem::getOrderId));
    
    // 一次查询获取所有用户
    Set<Long> userIds = orders.stream()
        .map(Order::getUserId)
        .collect(Collectors.toSet());
    Map<Long, User> userById = userRepository.findAllById(userIds)
        .stream()
        .collect(Collectors.toMap(User::getId, Function.identity()));
    
    // 组装结果
    return orders.stream().map(order -> {
        OrderDTO dto = new OrderDTO();
        dto.setOrder(order);
        dto.setItems(itemsByOrderId.getOrDefault(order.getId(), List.of()));
        dto.setUserName(userById.get(order.getUserId()).getName());
        return dto;
    }).collect(Collectors.toList());
}

这样只需要 3 次数据库查询，性能提升约 100 倍。

开发者：

感谢这个详细的建议！我已经修改为批量查询方式。

另外，我在 Repository 中添加了对应的批量查询方法：

@Repository
public interface OrderItemRepository extends JpaRepository<OrderItem, Long> {
    List<OrderItem> findByOrderIdIn(List<Long> orderIds);
}

经验总结​

  • 循环中的数据库查询是性能问题的常见来源
  • N+1 问题在高并发场景下会导致数据库压力剧增
  • 批量查询和内存组装是解决 N+1 的常用方法

案例 4：并发安全问题​

场景描述​

开发者实现了一个计数器功能，但未考虑并发场景。

提交的代码​

@Service
public class CounterService {
    
    private int count = 0;
    
    public void increment() {
        count++;
    }
    
    public int getCount() {
        return count;
    }
}

审查对话​

审查者：

🔴 阻塞：这段代码在多线程环境下不安全。

count++ 不是原子操作，它包含三个步骤：

  1. 读取 count 的值
  2. 值加 1
  3. 将新值写回 count

如果两个线程同时执行 increment()，可能会丢失更新。

根据场景，有几种修复方案：

方案 1：使用 AtomicInteger

@Service
public class CounterService {
    private AtomicInteger count = new AtomicInteger(0);
    
    public void increment() {
        count.incrementAndGet();
    }
    
    public int getCount() {
        return count.get();
    }
}

方案 2：如果需要分布式计数，使用 Redis

@Service
public class CounterService {
    @Autowired
    private RedisTemplate<String, Integer> redisTemplate;
    
    private static final String COUNTER_KEY = "counter:main";
    
    public void increment() {
        redisTemplate.opsForValue().increment(COUNTER_KEY);
    }
    
    public int getCount() {
        Integer value = redisTemplate.opsForValue().get(COUNTER_KEY);
        return value != null ? value : 0;
    }
}

开发者：

感谢指出！我使用 AtomicInteger 修复了这个问题。

但我想问一下：如果将来需要分布式部署，是否应该直接使用 Redis 方案？

审查者：

这是个好问题。

如果当前是单机部署，且近期没有分布式计划，使用 AtomicInteger 更简单，性能也更好。

如果分布式部署是明确的需求，或者不确定，那么使用 Redis 更稳妥。

建议：目前使用 AtomicInteger，在代码注释中说明"如果需要分布式部署，需要改用 Redis"。这样既保持简单，又为未来留有余地。

经验总结​

  • 共享变量的并发访问必须考虑线程安全
  • 选择同步方案时要考虑部署架构（单机 vs 分布式）
  • 适度的过度设计比事后重构更经济

案例 5：错误处理改进​

场景描述​

开发者在异常处理中只打印了日志，没有进行合适的错误处理。

提交的代码​

@Service
public class PaymentService {
    
    public void processPayment(Long orderId, BigDecimal amount) {
        try {
            // 调用支付网关
            PaymentResult result = paymentGateway.charge(amount);
            
            // 更新订单状态
            orderRepository.updateStatus(orderId, "PAID");
            
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

审查对话​

审查者：

🔴 阻塞：异常处理存在多个问题：

  1. 吞掉了异常：e.printStackTrace() 只是打印到标准错误，异常被吞掉了，调用者不知道发生了错误

  2. 没有更新订单状态：支付失败时订单状态没有被正确标记

  3. 使用 e.printStackTrace()：应该使用日志框架，而不是直接打印到控制台

  4. 捕获范围太广：不应该捕获 Exception，应该捕获具体的异常类型

建议修改：

@Service
@Slf4j
public class PaymentService {
    
    public void processPayment(Long orderId, BigDecimal amount) {
        try {
            PaymentResult result = paymentGateway.charge(amount);
            orderRepository.updateStatus(orderId, "PAID");
            
        } catch (PaymentGatewayException e) {
            // 业务异常：支付网关问题
            log.error("支付网关调用失败，订单ID: {}, 金额: {}", orderId, amount, e);
            orderRepository.updateStatus(orderId, "PAYMENT_FAILED");
            throw new PaymentException("支付失败，请稍后重试", e);
            
        } catch (NetworkException e) {
            // 网络异常：可能需要重试
            log.warn("支付网关网络异常，订单ID: {}", orderId, e);
            orderRepository.updateStatus(orderId, "PAYMENT_PENDING");
            throw new PaymentException("支付处理中，请查询订单状态", e);
        }
    }
}

开发者：

非常感谢这个详细的审查意见！我根据建议修改了代码。

另外，我添加了一个定时任务来处理 PENDING 状态的订单：

@Scheduled(fixedDelay = 300000)  // 每5分钟执行一次
public void reconcilePendingPayments() {
    List<Order> pendingOrders = orderRepository.findByStatus("PAYMENT_PENDING");
    for (Order order : pendingOrders) {
        // 查询支付网关确认支付状态
        reconcilePayment(order);
    }
}

审查者：

很好的补充！这个对账机制让系统更加健壮。

💡 建议：对于定时任务，考虑添加以下改进：

  1. 添加执行时间限制，避免长时间运行
  2. 添加分布式锁，避免多实例重复执行
  3. 记录执行日志，方便监控和排查

经验总结​

  • 异常处理是系统健壮性的关键，不能简单吞掉异常
  • 不同类型的异常应该有不同的处理策略
  • 对于涉及外部调用的操作，需要有对账或重试机制

案例 6：代码可读性改进​

场景描述​

开发者实现了一个复杂的业务逻辑，代码可读性较差。

提交的代码​

public BigDecimal calculate(Order order) {
    BigDecimal r = BigDecimal.ZERO;
    for (OrderItem i : order.getItems()) {
        BigDecimal p = i.getPrice().multiply(new BigDecimal(i.getQuantity()));
        if (i.getCategory().equals("ELECTRONICS")) {
            p = p.multiply(new BigDecimal("0.9"));
        } else if (i.getCategory().equals("BOOKS")) {
            p = p.multiply(new BigDecimal("0.8"));
        }
        if (order.getUser().getLevel() > 2) {
            p = p.multiply(new BigDecimal("0.95"));
        }
        r = r.add(p);
    }
    if (r.compareTo(new BigDecimal("1000")) > 0) {
        r = r.subtract(new BigDecimal("50"));
    }
    return r;
}

审查对话​

审查者：

💡 建议：这段代码的可读性可以改进。

当前问题：

  1. 魔法数字（0.9、0.8、0.95 等）没有说明含义
  2. 变量名太短（r、p、i），难以理解
  3. 复杂的逻辑没有注释
  4. 折扣规则分散在循环中，难以维护

建议重构：

private static final BigDecimal ELECTRONICS_DISCOUNT = new BigDecimal("0.9");  // 电子类商品9折
private static final BigDecimal BOOKS_DISCOUNT = new BigDecimal("0.8");         // 图书类商品8折
private static final BigDecimal VIP_DISCOUNT = new BigDecimal("0.95");          // VIP会员额外95折
private static final BigDecimal FULL_REDUCTION_THRESHOLD = new BigDecimal("1000");  // 满1000减50
private static final BigDecimal FULL_REDUCTION_AMOUNT = new BigDecimal("50");

public BigDecimal calculateTotalPrice(Order order) {
    BigDecimal totalPrice = BigDecimal.ZERO;
    
    for (OrderItem item : order.getItems()) {
        BigDecimal itemTotal = calculateItemPrice(item, order.getUser());
        totalPrice = totalPrice.add(itemTotal);
    }
    
    // 满减优惠
    totalPrice = applyFullReduction(totalPrice);
    
    return totalPrice;
}

private BigDecimal calculateItemPrice(OrderItem item, User user) {
    BigDecimal basePrice = item.getPrice().multiply(new BigDecimal(item.getQuantity()));
    
    // 应用品类折扣
    BigDecimal discountedPrice = applyCategoryDiscount(basePrice, item.getCategory());
    
    // 应用VIP折扣
    if (user.isVip()) {
        discountedPrice = discountedPrice.multiply(VIP_DISCOUNT);
    }
    
    return discountedPrice;
}

private BigDecimal applyCategoryDiscount(BigDecimal price, String category) {
    switch (category) {
        case "ELECTRONICS":
            return price.multiply(ELECTRONICS_DISCOUNT);
        case "BOOKS":
            return price.multiply(BOOKS_DISCOUNT);
        default:
            return price;
    }
}

private BigDecimal applyFullReduction(BigDecimal total) {
    if (total.compareTo(FULL_REDUCTION_THRESHOLD) > 0) {
        return total.subtract(FULL_REDUCTION_AMOUNT);
    }
    return total;
}

开发者：

非常感谢这个重构建议！我按照你的方式重构了代码。

另外我把折扣规则提取到了配置文件中，方便后续调整：

# application.yml
discount:
  categories:
    ELECTRONICS: 0.9
    BOOKS: 0.8
  vip: 0.95
  full-reduction:
    threshold: 1000
    amount: 50

审查者：

很好的改进！将折扣规则配置化让系统更灵活。

📝 可选：如果折扣规则可能频繁变化，可以考虑放到数据库或配置中心，支持动态更新。

经验总结​

  • 可读性差的代码是技术债务，会增加后续维护成本
  • 提取方法、使用有意义的命名、添加常量都能提升可读性
  • 折扣、费率等业务规则应该配置化，方便调整

案例 7：测试缺失​

场景描述​

开发者提交了一个重要的业务功能，但没有编写测试。

提交的代码​

@Service
public class TransferService {
    
    @Autowired
    private AccountRepository accountRepository;
    
    @Transactional
    public void transfer(String fromAccountId, String toAccountId, BigDecimal amount) {
        Account from = accountRepository.findById(fromAccountId);
        Account to = accountRepository.findById(toAccountId);
        
        if (from.getBalance().compareTo(amount) < 0) {
            throw new InsufficientBalanceException("余额不足");
        }
        
        from.setBalance(from.getBalance().subtract(amount));
        to.setBalance(to.getBalance().add(amount));
        
        accountRepository.save(from);
        accountRepository.save(to);
    }
}

审查对话​

审查者：

🔴 阻塞：这是一个涉及资金转账的核心功能，缺少单元测试是不可接受的。

需要测试的场景包括：

  1. 正常转账成功
  2. 余额不足时抛出异常
  3. 转出账户不存在
  4. 转入账户不存在
  5. 金额为负数或零

建议添加测试：

@SpringBootTest
class TransferServiceTest {
    
    @Autowired
    private TransferService transferService;
    
    @MockBean
    private AccountRepository accountRepository;
    
    @Test
    void transfer_successful() {
        // Given
        Account from = new Account("A001", new BigDecimal("1000"));
        Account to = new Account("A002", new BigDecimal("500"));
        
        when(accountRepository.findById("A001")).thenReturn(from);
        when(accountRepository.findById("A002")).thenReturn(to);
        
        // When
        transferService.transfer("A001", "A002", new BigDecimal("300"));
        
        // Then
        assertThat(from.getBalance()).isEqualByComparingTo("700");
        assertThat(to.getBalance()).isEqualByComparingTo("800");
    }
    
    @Test
    void transfer_insufficientBalance_throwsException() {
        // Given
        Account from = new Account("A001", new BigDecimal("100"));
        Account to = new Account("A002", new BigDecimal("500"));
        
        when(accountRepository.findById("A001")).thenReturn(from);
        when(accountRepository.findById("A002")).thenReturn(to);
        
        // When & Then
        assertThatThrownBy(() -> 
            transferService.transfer("A001", "A002", new BigDecimal("200"))
        ).isInstanceOf(InsufficientBalanceException.class);
    }
    
    @Test
    void transfer_accountNotFound_throwsException() {
        when(accountRepository.findById("A001")).thenReturn(null);
        
        assertThatThrownBy(() -> 
            transferService.transfer("A001", "A002", new BigDecimal("100"))
        ).isInstanceOf(AccountNotFoundException.class);
    }
}

开发者：

我补充了完整的单元测试，覆盖了你提到的所有场景。

另外，我发现了一个之前没有处理的问题：如果转入账户不存在，也应该抛出异常。我已经在代码中添加了这个检查。

经验总结​

  • 核心业务逻辑必须有测试覆盖
  • 测试应该覆盖正常路径和边界情况
  • 编写测试的过程往往能发现代码中的问题

小结​

通过这些实战案例，我们可以看到：

  1. 安全问题是阻塞性的：SQL 注入、XSS 等安全漏洞必须在合并前修复
  2. 并发和空值是常见问题：需要在审查时特别关注
  3. 性能问题容易被忽视：N+1 查询、内存泄漏等问题需要审查者有经验
  4. 可读性影响维护成本：不要求完美，但应该持续改进
  5. 测试是质量的保障：核心功能必须有测试覆盖

良好的代码审查需要审查者具备扎实的技术功底和丰富的项目经验。通过不断实践和总结，每个人都可以成为优秀的审查者。

案例 8：过度设计问题​

场景描述​

开发者为一个简单的功能引入了复杂的抽象层，导致代码难以理解和维护。

提交的代码​

// 开发者为简单的日志记录功能创建了一整套抽象体系
public interface LogStrategy {
    void log(String message);
}

public class ConsoleLogStrategy implements LogStrategy {
    @Override
    public void log(String message) {
        System.out.println(message);
    }
}

public class FileLogStrategy implements LogStrategy {
    @Override
    public void log(String message) {
        // 写入文件...
    }
}

public class LogStrategyFactory {
    private Map<String, LogStrategy> strategies = new HashMap<>();
    
    public LogStrategy getStrategy(String type) {
        return strategies.get(type);
    }
}

public class LogManager {
    private LogStrategyFactory factory;
    
    public void log(String type, String message) {
        LogStrategy strategy = factory.getStrategy(type);
        strategy.log(message);
    }
}

审查对话​

审查者：

💡 建议：这段代码存在过度设计的问题。

当前的需求只是简单的日志记录，但引入了策略模式、工厂模式等多层抽象。这带来了几个问题：

  1. 增加了理解成本：新成员需要理解多个类之间的关系
  2. 增加了维护成本：简单的修改需要改动多个文件
  3. 违反 YAGNI 原则：目前只需要控制台日志，其他策略是假设的未来需求

Google 工程实践指南明确指出：审查者应该特别警惕过度设计。鼓励开发者解决他们知道现在需要解决的问题，而不是推测将来可能需要解决的问题。

对于当前需求，简单的实现就足够：

@Service
@Slf4j
public class SimpleLogger {
    
    public void log(String message) {
        log.info(message);
    }
}

如果未来确实需要多种日志输出方式，再进行抽象也不迟。到那时，我们对需求会有更清晰的理解，抽象也会更有针对性。

开发者：

我理解了。我确实是根据"将来可能需要"来设计的，但当前需求确实很简单。

我简化了实现，使用 SLF4J 直接记录日志。如果将来需要扩展，再考虑抽象。

审查者：

很好！记住一个原则：让代码足够简单，而不是尽可能通用。

过度设计的代码比设计不足的代码更难重构，因为它需要先简化再扩展。

经验总结​

  • 只解决当前确定的需求，不为假设的未来需求过度设计
  • 简单的实现比复杂的抽象更容易理解和维护
  • 当需求明确时再进行抽象，抽象会更有针对性

案例 9：注释问题​

场景描述​

开发者添加了大量注释，但注释质量不高，反而影响了代码可读性。

提交的代码​

public class OrderProcessor {
    
    // 处理订单
    public void process(Order order) {
        // 检查订单是否为空
        if (order == null) {
            // 抛出异常
            throw new IllegalArgumentException("订单不能为空");
        }
        
        // 获取订单项列表
        List<OrderItem> items = order.getItems();
        
        // 遍历订单项
        for (OrderItem item : items) {
            // 获取商品价格
            BigDecimal price = item.getPrice();
            // 获取商品数量
            int quantity = item.getQuantity();
            // 计算小计
            BigDecimal subtotal = price.multiply(new BigDecimal(quantity));
            // 打印小计
            System.out.println("小计：" + subtotal);
        }
    }
}

审查对话​

审查者：

💡 建议：这段代码的注释存在问题。

问题分析：

  1. 注释解释了代码做什么：如 "检查订单是否为空"、"遍历订单项" 这些注释只是重复代码的含义，没有增加价值

  2. 应该解释为什么：好的注释应该解释代码存在的原因，而不是代码在做什么

  3. 代码应该自解释：如果代码足够清晰，就不需要注释

Google 工程实践指南指出：注释在解释为什么存在时很有用，不应该解释代码正在做什么。如果代码不够清晰以至于无法解释自己，应该简化代码。

改进建议：

public class OrderProcessor {
    
    /**
     * 处理订单，计算并打印每个商品的小计金额。
     * 
     * 注意：此方法用于调试目的，生产环境应使用异步日志记录。
     */
    public void process(Order order) {
        validateOrder(order);
        
        for (OrderItem item : order.getItems()) {
            BigDecimal subtotal = calculateSubtotal(item);
            log.info("商品 {}: 小计 {}", item.getProductName(), subtotal);
        }
    }
    
    private void validateOrder(Order order) {
        if (order == null) {
            throw new IllegalArgumentException("订单不能为空");
        }
    }
    
    private BigDecimal calculateSubtotal(OrderItem item) {
        return item.getPrice().multiply(new BigDecimal(item.getQuantity()));
    }
}

改进后的版本：

  • 使用方法名让代码自解释
  • 类级别的注释说明了方法的用途和注意事项
  • 删除了重复代码含义的行内注释

开发者：

感谢这个指导！我之前以为注释越多越好，现在明白了注释应该解释"为什么"而不是"做什么"。

我重构了代码，让方法名更清晰，代码基本可以自解释了。

经验总结​

  • 注释应该解释"为什么"而不是"做什么"
  • 代码应该尽量自解释，减少对注释的依赖
  • 方法名和变量名是最好的"注释"
  • 过多的低质量注释反而增加阅读负担

案例 10：敏感信息泄露​

场景描述​

开发者在日志中直接打印了用户的敏感信息。

提交的代码​

@Service
@Slf4j
public class UserService {
    
    public User login(String username, String password) {
        log.info("用户登录请求：username={}, password={}", username, password);
        
        User user = userRepository.findByUsername(username);
        
        if (user != null && passwordEncoder.matches(password, user.getPassword())) {
            log.info("登录成功：user={}", user);
            return user;
        }
        
        log.warn("登录失败：username={}, password={}", username, password);
        return null;
    }
}

审查对话​

审查者：

🔴 阻塞：严重的敏感信息泄露问题！

代码中存在多个安全问题：

  1. 密码被记录到日志：password={} 会将用户密码明文写入日志文件
  2. 用户对象被完整打印：可能包含手机号、身份证等敏感信息
  3. 登录失败也记录密码：攻击者可以通过日志获取密码

日志文件可能被多人访问（开发、运维、审计），敏感信息一旦泄露后果严重。

修复建议：

@Service
@Slf4j
public class UserService {
    
    public User login(String username, String password) {
        log.info("用户登录请求：username={}", username);
        
        User user = userRepository.findByUsername(username);
        
        if (user != null && passwordEncoder.matches(password, user.getPassword())) {
            log.info("登录成功：userId={}, username={}", user.getId(), username);
            return user;
        }
        
        log.warn("登录失败：username={}", username);
        return null;
    }
}

更进一步，建议对敏感字段进行脱敏处理：

public class User {
    
    @ToString.Exclude  // Lombok 自动排除
    private String password;
    
    public String getPhone() {
        return phone;  // 原始值
    }
    
    public String getPhoneMasked() {
        // 返回脱敏后的手机号：138****8888
        return phone.replaceAll("(\\d{3})\\d{4}(\\d{4})", "$1****$2");
    }
}

开发者：

这是一个严重的问题，我立即修复。

我还检查了其他地方的日志，发现还有几处类似的问题，一并修复了。同时添加了代码审查检查清单中关于敏感信息的项目。

审查者：

做得很好！主动排查其他地方的问题体现了对安全的重视。

建议在团队内分享这个案例，提高大家的安全意识。

经验总结​

  • 密码、手机号、身份证等敏感信息绝对不能出现在日志中
  • 日志记录只应包含必要的业务信息
  • 对需要记录的敏感字段进行脱敏处理
  • 定期审查日志内容，确保没有敏感信息泄露

案例 11：PR 过大问题​

场景描述​

开发者提交了一个超过 2000 行变更的 PR，包含多个不相关的修改。

PR 描述​

本次更新包括：
1. 用户登录功能
2. 数据库连接池优化
3. 日志框架升级
4. 代码格式调整
5. 单元测试补充

审查对话​

审查者：

❓ 疑问：这个 PR 包含了多个不相关的变更，能否解释一下为什么要合并在一个 PR 中？

开发者：

我觉得这些改动都相关，而且这样可以一次性完成所有优化。

审查者：

我理解你的想法，但这样做会带来几个问题：

  1. 审查困难：2000 多行变更，审查者难以逐行仔细检查
  2. 难以回滚：如果其中一个改动有问题，整个 PR 都需要回滚
  3. 责任不清：出现问题时，难以定位是哪个改动导致的
  4. 阻塞其他工作：如果某个小改动需要调整，整个 PR 都不能合并

研究表明，超过 400 行的 PR，审查发现问题的能力会显著下降。Google 建议将大型变更拆分为多个小 PR。

建议拆分方案：

PR内容预计行数
PR-1代码格式调整（无逻辑变更）~500 行
PR-2日志框架升级~200 行
PR-3数据库连接池优化~300 行
PR-4用户登录功能 + 测试~400 行

这样每个 PR 都有明确的目标，审查者可以更有针对性地审查，也能更快地合并。

开发者：

感谢建议！我来拆分这个 PR。代码格式调整我先单独提交，因为这部分没有逻辑变更，可以快速审查通过。

审查者：

很好！一个经验法则：每个 PR 应该只解决一个问题。

小 PR 的好处：

  • 审查更快速
  • 问题更容易发现
  • 回滚风险更低
  • 合并更顺利

经验总结​

  • 单个 PR 控制在 400 行以内为佳
  • 一个 PR 只解决一个问题
  • 不相关的改动应该拆分为独立的 PR
  • 代码格式调整应该独立提交，不与功能改动混合

小结​

通过这些实战案例，我们可以看到代码审查中的常见问题和处理方式：

安全相关：

  • SQL 注入、XSS 等漏洞必须立即修复
  • 敏感信息不能出现在日志或错误信息中
  • 用户输入必须经过验证和清理

代码质量：

  • 空值处理是常见缺陷来源
  • 并发安全需要特别关注
  • 错误处理应该完善且有意义
  • 代码可读性影响长期维护成本

设计决策：

  • 避免过度设计，只解决当前需求
  • 注释应该解释"为什么"而不是"做什么"
  • PR 应该保持小型，专注于单一变更

测试保障：

  • 核心功能必须有测试覆盖
  • 测试应该覆盖正常路径和边界情况
  • 编写测试的过程往往能发现代码问题

代码审查是一门需要不断实践的技能。通过积累经验，审查者能够更快速地发现问题，更有效地与开发者沟通，最终提升整个团队的代码质量。

练习​

  1. 回顾你最近提交或审查的 PR，找出可以改进的地方
  2. 选择一个你不太熟悉的代码库，尝试进行代码审查
  3. 与团队成员讨论你们的代码审查流程，找出可以改进的地方
  4. 尝试用本文中的案例作为检查清单，审查一段代码