java-code-review

Compare original and translation side by side

🇺🇸

Original

English

🇨🇳

Translation

Chinese

Java Code Review Skill

Java代码审查技能

Systematic code review checklist for Java projects.

Java项目的系统性代码审查清单。

When to Use

使用场景

User says "review this code" / "check this PR" / "code review"
Before merging a PR
After implementing a feature

用户说"审查这段代码"/"检查这个PR"/"代码审查"
合并PR之前
实现功能之后

Review Strategy

审查策略

Quick scan - Understand intent, identify scope
Checklist pass - Go through each category below
Summary - List findings by severity (Critical → Minor)

快速扫描 - 理解意图，确定范围
清单检查 - 逐一检查以下每个类别
总结 - 按严重程度列出发现的问题（严重→次要）

Output Format

输出格式

markdown

undefined

markdown

undefined

Code Review: [file/feature name]

Critical

[Issue description + line reference + suggestion]

[Issue description + line reference + suggestion]

Improvements

[Suggestion + rationale]

[Suggestion + rationale]

Minor/Style

[Nitpicks, optional improvements]

[Nitpicks, optional improvements]

Good Practices Observed

[Positive feedback - important for morale]

---

[Positive feedback - important for morale]

---

Review Checklist

审查清单

1. Null Safety

1. 空值安全

Check for:

java

// ❌ 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();

Flags:

Chained method calls without null checks
Missing
```
@Nullable
```
/
```
@NonNull
```
annotations on public APIs
```
Optional.get()
```
without
```
isPresent()
```
check
Returning
```
null
```
from methods that could return
```
Optional
```
or empty collection

Suggest:

Use
```
Optional
```
for return types that may be absent
Use
```
Objects.requireNonNull()
```
for constructor/method params
Return empty collections instead of null:
```
Collections.emptyList()
```

检查要点：

java

// ❌ 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
```
注解
未使用
```
isPresent()
```
检查就调用
```
Optional.get()
```
本可返回Optional或空集合的方法却返回null

建议：

对可能为空的返回类型使用
```
Optional
```
对构造函数/方法参数使用
```
Objects.requireNonNull()
```
返回空集合而非null：
```
Collections.emptyList()
```

2. Exception Handling

2. 异常处理

Check for:

java

// ❌ 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);
}

Flags:

Empty catch blocks
Catching
```
Exception
```
or
```
Throwable
```
broadly
Losing original exception (not chaining)
Using exceptions for flow control
Checked exceptions leaking through API boundaries

Suggest:

Log with context AND stack trace
Use specific exception types
Chain exceptions with
```
cause
```
Consider custom exceptions for domain errors

检查要点：

java

// ❌ 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. Collections & Streams

3. 集合与流

Check for:

java

// ❌ 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));

Flags:

Modifying collections during iteration
Overusing streams for simple operations
Assuming
```
Collectors.toList()
```
returns mutable list
Not using
```
List.of()
```
,
```
Set.of()
```
,
```
Map.of()
```
for immutable collections
Parallel streams without understanding implications

Suggest:

```
List.copyOf()
```
for defensive copies
```
removeIf()
```
instead of iterator removal
Streams for transformations, loops for side effects

检查要点：

java

// ❌ 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. Concurrency

4. 并发

Check for:

java

// ❌ 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();
        }
    }
}

Flags:

Shared mutable state without synchronization
Check-then-act patterns without atomicity
Missing
```
volatile
```
on shared variables
Synchronized on non-final objects
Thread-unsafe lazy initialization

Suggest:

Prefer immutable objects
Use
```
java.util.concurrent
```
classes
```
AtomicReference
```
,
```
AtomicInteger
```
for simple cases
Consider
```
@ThreadSafe
```
/
```
@NotThreadSafe
```
annotations

检查要点：

java

// ❌ 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 Idioms

5. Java惯用写法

equals/hashCode:

java

// ❌ 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:

java

// ❌ 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 + "'}";
}

Builders:

java

// ✅ For classes with many optional parameters
User user = User.builder()
    .name("John")
    .email("john@example.com")
    .build();

Flags:

```
equals
```
without
```
hashCode
```
Mutable fields in
```
hashCode
```
Missing
```
toString
```
on domain objects
Constructors with > 3-4 parameters (suggest builder)
Not using
```
instanceof
```
pattern matching (Java 16+)

equals/hashCode：

java

// ❌ 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：

java

// ❌ 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 + "'}";
}

构建器：

java

// ✅ For classes with many optional parameters
User user = User.builder()
    .name("John")
    .email("john@example.com")
    .build();

警示点：

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

6. Resource Management

6. 资源管理

Check for:

java

// ❌ 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
}

Flags:

Not using try-with-resources for
```
Closeable
```
/
```
AutoCloseable
```
Resources opened but not in try-with-resources
Database connections/statements not properly closed

检查要点：

java

// ❌ 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 Design

7. API设计

Check for:

java

// ❌ 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");
}

Flags:

Boolean parameters (prefer enums)
Methods with > 3 parameters (consider parameter object)
Inconsistent null handling across similar methods
Missing validation on public API inputs

检查要点：

java

// ❌ 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. Performance Considerations

8. 性能考量

Check for:

java

// ❌ 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);

Flags:

String concatenation in loops
Regex compilation in loops
N+1 query patterns
Creating objects in tight loops that could be reused
Not using primitive streams (
```
IntStream
```
,
```
LongStream
```
)

检查要点：

java

// ❌ 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. Testing Hints

9. 测试提示

Suggest tests for:

Null inputs
Empty collections
Boundary values
Exception cases
Concurrent access (if applicable)

建议测试场景：

空输入
空集合
边界值
异常场景
并发访问（如适用）

Severity Guidelines

严重程度指南

Severity	Criteria
Critical	Security vulnerability, data loss risk, production crash
High	Bug likely, significant performance issue, breaks API contract
Medium	Code smell, maintainability issue, missing best practice
Low	Style, minor optimization, suggestion

严重程度	判定标准
严重	安全漏洞、数据丢失风险、生产环境崩溃
高	可能存在Bug、显著性能问题、违反API契约
中	代码异味、可维护性问题、缺失最佳实践
低	风格问题、次要优化、建议性内容

Token Optimization

Token优化建议

Focus on changed lines (use
```
git diff
```
)
Don't repeat obvious issues - group similar findings
Reference line numbers, not full code quotes
Skip files that are auto-generated or test fixtures

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

Quick Reference Card

快速参考卡片

Category	Key Checks
Null Safety	Chained calls, Optional misuse, null returns
Exceptions	Empty catch, broad catch, lost stack trace
Collections	Modification during iteration, stream vs loop
Concurrency	Shared mutable state, check-then-act
Idioms	equals/hashCode pair, toString, builders
Resources	try-with-resources, connection leaks
API	Boolean params, null handling, validation
Performance	String concat, regex in loop, N+1

类别	关键检查项
空值安全	链式调用、Optional误用、null返回
异常处理	空catch块、宽泛捕获、丢失堆栈跟踪
集合	迭代时修改、流与循环选择
并发	共享可变状态、检查-执行模式
惯用写法	equals/hashCode配对、toString、构建器
资源管理	try-with-resources、连接泄漏
API设计	布尔参数、空值处理、验证
性能	字符串拼接、循环中正则、N+1查询