code-quality

代码质量审查技能

结合整洁代码原则、API 设计和 Java 最佳实践，系统地审查代码。

适用场景

• “审查这段代码” / “代码审查” / “检查这个 PR”
• “重构” / “清理这段代码” / “提高可读性”
• “审查 API” / “检查端点” / “审查 REST”
• 合并 PR 或发布 API 更改之前

审查策略

1. 快速浏览 - 理解意图，确定范围
2. 逐项检查 - 应用下方相关类别的检查项
3. 总结 - 按严重程度列出发现（严重 → 轻微 → 良好）

---

整洁代码原则

DRY - 不要重复自己

违反示例：

// ❌ Duplicated validation logic
public void createUser(UserRequest req) {
    if (req.getEmail() == null || !req.getEmail().contains("@")) {
        throw new ValidationException("Invalid email");
    }
}

public void updateUser(UserRequest req) {
    if (req.getEmail() == null || !req.getEmail().contains("@")) {
        throw new ValidationException("Invalid email");
    }
}

修正：

// ✅ Single source of truth
public class EmailValidator {
    public void validate(String email) {
        if (email == null || !email.contains("@")) {
            throw new ValidationException("Invalid email");
        }
    }
}

KISS - 保持简单

违反示例：

// ❌ Over-engineered
public interface UserFactory {
    User createUser();
}
public class ConcreteUserFactory implements UserFactory {
    public User createUser() { return new User(); }
}

修正：

// ✅ Simple
public User createUser() { return new User(); }

YAGNI - 你不会需要它

违反示例：

// ❌ Premature abstraction
public class ConfigurableUserServiceFactoryProvider { }

修正：

// ✅ Implement when actually needed
public class UserService { }

---

API 契约审查

HTTP 动词语义

动词	用途	幂等	安全
GET	获取资源	是	是
POST	创建新资源	否	否
PUT	替换整个资源	是	否
PATCH	部分更新	否*	否
DELETE	删除资源	是	否

常见错误：

// ❌ POST for retrieval
@PostMapping("/users/search")
public List<User> search(@RequestBody SearchCriteria criteria) { }

// ✅ GET with query params
@GetMapping("/users")
public List<User> search(@RequestParam String name) { }

// ❌ GET for state change
@GetMapping("/users/{id}/activate")
public void activate(@PathVariable Long id) { }

// ✅ POST/PATCH for state change
@PostMapping("/users/{id}/activate")
public ResponseEntity<Void> activate(@PathVariable Long id) { }

API 版本控制

// ✅ URL path versioning (recommended)
@RestController
@RequestMapping("/api/v1/users")
public class UserControllerV1 { }

// ❌ No versioning
@RequestMapping("/users")  // Breaking changes affect all clients

响应状态码

代码	使用场景	示例
200 OK	GET/PUT/PATCH 成功	找到资源
201 Created	POST 成功	已创建新资源
204 No Content	DELETE 成功	已删除资源
400 Bad Request	验证失败	输入无效
404 Not Found	资源不存在	未找到用户
409 Conflict	状态冲突	邮箱重复
500 Server Error	意外错误	数据库宕机

DTO 与实体暴露

// ❌ Exposing JPA entity
@GetMapping("/{id}")
public User getUser(@PathVariable Long id) {
    return userRepository.findById(id).get();  // Exposes internals, N+1 risk
}

// ✅ Use DTO
@GetMapping("/{id}")
public UserResponse getUser(@PathVariable Long id) {
    return userService.findById(id);  // Returns DTO
}

---

Java 代码审查清单

空值安全

检查项：

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

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

// ✅ Safe with early return
if (user.getName() == null) return "";
return user.getName().toUpperCase();

问题标记：

• 链式调用未进行空值检查
• 调用 Optional.get() 前未调用 isPresent()
• 返回 null，而非 Optional 或空集合
• 公共 API 缺少 @Nullable/@NonNull

异常处理

检查项：

// ❌ Swallowing exceptions
try {
    process();
} catch (Exception e) { }  // Silent failure

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

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

问题标记：

• 空的 catch 块
• 捕获 Exception 或 Throwable（范围过宽）
• 未记录异常日志
• 创建新异常时未保留原始原因

资源管理

检查项：

// ❌ Resource leak
FileInputStream fis = new FileInputStream(file);
String content = read(fis);
fis.close();  // Won't execute if read() throws

// ✅ Try-with-resources
try (FileInputStream fis = new FileInputStream(file)) {
    return read(fis);
}  // Auto-closed

事务边界

检查项：

// ❌ Missing transaction
public void createUser(UserRequest request) {
    User user = new User();
    userRepository.save(user);
    roleRepository.save(new Role(user));  // Two separate transactions
}

// ✅ Proper transaction
@Transactional
public void createUser(UserRequest request) {
    User user = new User();
    userRepository.save(user);
    roleRepository.save(new Role(user));  // Single atomic transaction
}

命名约定

良好示例：

// ✅ Clear intent
public List<User> findActiveUsersByRole(String role) { }
public boolean isEmailValid(String email) { }
public void activateUser(Long userId) { }

不良示例：

// ❌ Unclear
public List<User> get(String s) { }
public boolean check(String str) { }
public void doStuff(Long id) { }

性能

检查项：

// ❌ N+1 query problem
List<User> users = userRepository.findAll();
for (User user : users) {
    List<Order> orders = orderRepository.findByUserId(user.getId());  // N queries
}

// ✅ Join fetch
@Query("SELECT u FROM User u LEFT JOIN FETCH u.orders")
List<User> findAllWithOrders();

// ❌ Loading all data
List<User> allUsers = userRepository.findAll();  // Could be millions

// ✅ Pagination
Page<User> users = userRepository.findAll(PageRequest.of(0, 20));

---

审查输出格式

## Code Review: [Component/Feature Name]

### Critical Issues
- **Null safety violation** (UserService.java:42) - `user.getName().toUpperCase()` can NPE. Use Optional or null check.
- **Resource leak** (FileHandler.java:15) - FileInputStream not closed. Use try-with-resources.

### Important Improvements
- **API design** - POST used for idempotent update (UserController.java:28). Use PUT instead.
- **Transaction missing** - Multi-step operation needs @Transactional (OrderService.java:56).
- **N+1 query** - Loop fetches orders individually (line 89). Use JOIN FETCH.

### Code Smells
- **Long method** - extractUserData() is 80 lines. Consider extracting sub-methods.
- **Magic number** - Use named constant instead of `86400` (line 123).
- **Inconsistent naming** - Mix of camelCase and snake_case in variables.

### Good Practices Observed
- ✅ Constructor injection used throughout
- ✅ DTOs properly separate from entities
- ✅ Comprehensive validation on all endpoints
- ✅ Good test coverage (87%)

---

快速参考：问题标记

类别	警示项
空值安全	链式调用、Optional.get()、返回 null
异常	空 catch 块、范围过宽的 catch、丢失堆栈跟踪
资源	手动调用 close()、缺少 try-with-resources
API 设计	HTTP 动词错误、无版本控制、暴露实体
事务	多步写入未使用 @Transactional
性能	N+1 查询、加载全部数据、缺少索引
整洁代码	代码重复、魔法数字、名称不明确
不使用 Lombok	任何 @Data、@Getter、@Setter、@Builder、@Slf4j、@RequiredArgsConstructor — 使用显式代码

---

严重程度

• 严重 - 安全问题、数据丢失、崩溃风险 → 合并前必须修复
• 重要 - 性能、可维护性、正确性 → 应当修复
• 代码异味 - 风格、复杂度、小问题 → 可以改进
• 良好 - 肯定性反馈，以强化良好实践