java-code-review

Java 代码审查技能

适用于 Java 项目的系统化代码审查清单。

何时使用

• 用户说“审查这段代码”／“检查这个 PR”／“代码审查”
• 合并 PR 前
• 实现功能后

审查策略

1. 快速浏览 - 理解意图，确定范围
2. 逐项检查 - 检查以下各个类别
3. 总结 - 按严重程度列出发现的问题（严重 → 轻微）

输出格式

## Code Review: [file/feature name]

### Critical
- [Issue description + line reference + suggestion]

### Improvements
- [Suggestion + rationale]

### Minor/Style
- [Nitpicks, optional improvements]

### Good Practices Observed
- [Positive feedback - important for morale]

---

审查清单

1. 空值安全

检查：

// ❌ NPE risk
String name = user.getName().toUpperCase();

// ✅ Safe
String name = Optional.ofNullable(user.getName())
    .map(String::toUpperCase)
    .orElse("");

// ✅ Also safe (early return)
if (user.getName() == null) {
    return "";
}
return user.getName().toUpperCase();

注意事项：

• 链式方法调用未进行空值检查
• 公共 API 缺少 @Nullable／@NonNull 注解
• 调用 Optional.get() 前未检查 isPresent()
• 方法可能返回 Optional 或空集合，却返回了 null

建议：

• 对可能缺失的返回值使用 Optional
• 使用 Objects.requireNonNull() 检查构造函数／方法参数
• 返回空集合而不是 null：Collections.emptyList()

2. 异常处理

检查：

// ❌ Swallowing exceptions
try {
    process();
} catch (Exception e) {
    // silently ignored
}

// ❌ Catching too broad
catch (Exception e) { }
catch (Throwable t) { }

// ❌ Losing stack trace
catch (IOException e) {
    throw new RuntimeException(e.getMessage());
}

// ✅ Proper handling
catch (IOException e) {
    log.error("Failed to process file: {}", filename, e);
    throw new ProcessingException("File processing failed", e);
}

注意事项：

• 空的 catch 块
• 宽泛地捕获 Exception 或 Throwable
• 丢失原始异常（未串联异常）
• 使用异常控制流程
• 受检异常泄漏到 API 边界之外

建议：

• 记录日志时附带上下文和堆栈跟踪
• 使用具体的异常类型
• 使用 cause 串联异常
• 对领域错误考虑使用自定义异常

3. 集合与流

检查：

// ❌ Modifying while iterating
for (Item item : items) {
    if (item.isExpired()) {
        items.remove(item);  // ConcurrentModificationException
    }
}

// ✅ Use removeIf
items.removeIf(Item::isExpired);

// ❌ Stream for simple operations
list.stream().forEach(System.out::println);

// ✅ Simple loop is cleaner
for (Item item : list) {
    System.out.println(item);
}

// ❌ Collecting to modify
List<String> names = users.stream()
    .map(User::getName)
    .collect(Collectors.toList());
names.add("extra");  // Might be immutable!

// ✅ Explicit mutable list
List<String> names = users.stream()
    .map(User::getName)
    .collect(Collectors.toCollection(ArrayList::new));

注意事项：

• 遍历期间修改集合
• 对简单操作过度使用流
• 假设 Collectors.toList() 会返回可变列表
• 不使用 List.of()、Set.of()、Map.of() 创建不可变集合
• 未了解并行流的影响便使用并行流

建议：

• 使用 List.copyOf() 创建防御性副本
• 使用 removeIf() 替代迭代器删除
• 使用流进行转换，使用循环处理副作用

4. 并发

检查：

// ❌ Not thread-safe
private Map<String, User> cache = new HashMap<>();

// ✅ Thread-safe
private Map<String, User> cache = new ConcurrentHashMap<>();

// ❌ Check-then-act race condition
if (!map.containsKey(key)) {
    map.put(key, computeValue());
}

// ✅ Atomic operation
map.computeIfAbsent(key, k -> computeValue());

// ❌ Double-checked locking (broken without volatile)
if (instance == null) {
    synchronized(this) {
        if (instance == null) {
            instance = new Instance();
        }
    }
}

注意事项：

• 共享可变状态未进行同步
• 检查后执行模式不具备原子性
• 共享变量缺少 volatile
• 对非 final 对象使用同步
• 线程不安全的延迟初始化

建议：

• 优先使用不可变对象
• 使用 java.util.concurrent 中的类
• 简单场景可使用 AtomicReference、AtomicInteger
• 考虑使用 @ThreadSafe／@NotThreadSafe 注解

5. Java 惯用写法

equals/hashCode：

// ❌ Only equals without hashCode
@Override
public boolean equals(Object o) { ... }
// Missing hashCode!

// ❌ Mutable fields in hashCode
@Override
public int hashCode() {
    return Objects.hash(id, mutableField);  // Breaks HashMap
}

// ✅ Use immutable fields, implement both
@Override
public boolean equals(Object o) {
    if (this == o) return true;
    if (!(o instanceof User user)) return false;
    return Objects.equals(id, user.id);
}

@Override
public int hashCode() {
    return Objects.hash(id);
}

toString：

// ❌ Missing - hard to debug
// No toString()

// ❌ Including sensitive data
return "User{password='" + password + "'}";

// ✅ Useful for debugging
@Override
public String toString() {
    return "User{id=" + id + ", name='" + name + "'}";
}

构建器：

// ✅ For classes with many optional parameters
User user = User.builder()
    .name("John")
    .email("[email protected]")
    .build();

注意事项：

• 实现了 equals 却没有实现 hashCode
• hashCode 中包含可变字段
• 领域对象缺少 toString
• 构造函数参数超过 3～4 个（建议使用构建器）
• 未使用 instanceof 模式匹配（Java 16+）

6. 资源管理

检查：

// ❌ Resource leak
FileInputStream fis = new FileInputStream(file);
// ... might throw before close

// ✅ Try-with-resources
try (FileInputStream fis = new FileInputStream(file)) {
    // ...
}

// ❌ Multiple resources, wrong order
try (BufferedWriter writer = new BufferedWriter(new FileWriter(file))) {
    // FileWriter might not be closed if BufferedWriter fails
}

// ✅ Separate declarations
try (FileWriter fw = new FileWriter(file);
     BufferedWriter writer = new BufferedWriter(fw)) {
    // Both properly closed
}

注意事项：

• 对 Closeable／AutoCloseable 未使用 try-with-resources
• 打开资源后未在 try-with-resources 中使用
• 数据库连接／语句未正确关闭

7. API 设计

检查：

// ❌ Boolean parameters
process(data, true, false);  // What do these mean?

// ✅ Use enums or builder
process(data, ProcessMode.ASYNC, ErrorHandling.STRICT);

// ❌ Returning null for "not found"
public User findById(Long id) {
    return users.get(id);  // null if not found
}

// ✅ Return Optional
public Optional<User> findById(Long id) {
    return Optional.ofNullable(users.get(id));
}

// ❌ Accepting null collections
public void process(List<Item> items) {
    if (items == null) items = Collections.emptyList();
}

// ✅ Require non-null, accept empty
public void process(List<Item> items) {
    Objects.requireNonNull(items, "items must not be null");
}

注意事项：

• 使用布尔参数（优先使用枚举）
• 方法参数超过 3 个（考虑使用参数对象）
• 类似方法的空值处理不一致
• 公共 API 输入缺少校验

8. 性能考量

检查：

// ❌ String concatenation in loop
String result = "";
for (String s : strings) {
    result += s;  // Creates new String each iteration
}

// ✅ StringBuilder
StringBuilder sb = new StringBuilder();
for (String s : strings) {
    sb.append(s);
}

// ❌ Regex compilation in loop
for (String line : lines) {
    if (line.matches("pattern.*")) { }  // Compiles regex each time
}

// ✅ Pre-compiled pattern
private static final Pattern PATTERN = Pattern.compile("pattern.*");
for (String line : lines) {
    if (PATTERN.matcher(line).matches()) { }
}

// ❌ N+1 in loops
for (User user : users) {
    List<Order> orders = orderRepo.findByUserId(user.getId());
}

// ✅ Batch fetch
Map<Long, List<Order>> ordersByUser = orderRepo.findByUserIds(userIds);

注意事项：

• 在循环中拼接字符串
• 在循环中编译正则表达式
• N+1 查询模式
• 在紧密循环中创建可复用的对象
• 未使用基本类型流（IntStream、LongStream）

9. 测试提示

建议测试：

• 空值输入
• 空集合
• 边界值
• 异常情况
• 并发访问（如适用）

---

严重程度指南

严重程度	标准
严重	安全漏洞、数据丢失风险、生产环境崩溃
高	很可能存在 bug、严重性能问题、违反 API 契约
中	代码异味、可维护性问题、缺少最佳实践
低	风格问题、轻微优化、建议

Token 优化

• 聚焦于变更的行（使用 git diff）
• 不要重复明显的问题——将相似发现归为一组
• 引用行号，而不是完整的代码片段
• 跳过自动生成的文件或测试夹具

快速参考卡

类别	关键检查项
空值安全	链式调用、误用 Optional、返回 null
异常	空 catch、过宽的 catch、丢失堆栈跟踪
集合	遍历时修改、流与循环
并发	共享可变状态、检查后再操作
惯用法	equals/hashCode 配对、toString、构建器
资源	try-with-resources、连接泄漏
API	布尔参数、空值处理、验证
性能	字符串拼接、循环中的正则表达式、N+1