24 KiB
Water 项目代码审查标准与流程
版本: 1.0 | 适用项目: water (IoT 智能灌溉设备管理系统) | 技术栈: Spring Boot 3.5 + Java 17 + MyBatis Plus + MQTT + Redis
目录
1. 代码审查标准
1.1 问题分级体系
所有审查意见必须标注严重等级,审查者不得给出无分级的意见:
| 等级 | 标记 | 含义 | 处理要求 |
|---|---|---|---|
| Blocker | 🔴 | 存在安全漏洞、数据丢失风险、破坏 API 契约、竞态条件 | 必须修复后才能合并 |
| Suggestion | 🟡 | 缺失输入校验、命名混乱、缺少测试、性能问题、重复代码 | 应当修复,特殊情况可延期 |
| Nit | 💭 | 风格不一致、命名微调、文档缺失 | 建议改进,不阻塞合并 |
1.2 架构规范
🔴 Blocker
- 禁止跨层调用:Controller 不得直接操作 Mapper/数据库;Service 不得返回
HttpEntity/ResponseEntity等 Web 层对象 - 禁止在 Controller 中写业务逻辑:Controller 只做参数接收、校验、调用 Service、组装返回值。逻辑超过 5 行的方法必须下沉到 Service
- 循环依赖必须根治:禁止用
@Lazy @Autowired掩盖循环依赖。如果存在循环依赖,说明模块划分有问题,应重构拆分
water 项目现状:
MqttCommandAckService中@Lazy @Autowired MqttClientManager是典型的循环依赖掩盖。审查中遇到此类代码应标记为 🟡 Suggestion,要求补充重构计划
- 单一职责:单个 Controller/Service 类行数不得超过 500 行。超过的必须拆分
water 项目现状:
AppController达 940 行,包含设备、排程、浇水记录、用户、统计、版本等全部业务接口,是典型的上帝类。新代码不得再向此类添加方法
🟡 Suggestion
- 接口与实现分离:Service 必须定义接口(
IXxxService)+ 实现(XxxServiceImpl),Controller 依赖接口 - 依赖注入使用构造器注入:使用
@RequiredArgsConstructor+final字段,禁止字段注入@Autowired(除@Lazy场景外) - 配置类使用
@ConfigurationProperties:禁止用@Value散落配置读取
1.3 命名规范
🔴 Blocker
- 禁止 Raw Type:集合必须使用泛型。
Map map = new HashMap()必须写为Map<String, Object> map = new HashMap<>()
water 项目现状:
AppController中存在Map retMap = new HashMap()、List l = new ArrayList<>()、List deviceList = new ArrayList()等大量 Raw Type 用法。新代码零容忍
- 变量名不得使用单字母:除循环变量
i/j/k外,变量名必须有意义。List l必须写为List<AppDeviceVo> deviceList
🟡 Suggestion
- 接口与实现的命名一致:注入字段名应与接口名对应。
IAppScheduleService注入为appScheduleService,不得出现appScheduleServiceimpl(小写 impl) - 布尔变量用 is/has/can 前缀:
flag→isSwitchSuccess、hasPermission - 常量全大写下划线:
DEVICE_STATUS_LOCK_PREFIX,禁止魔法值
💭 Nit
- 方法名动词开头:
getDeviceInfo、bindDevice、assertDeviceOwned - 包名全小写:不得使用大写字母或下划线
1.4 异常处理规范
🔴 Blocker
- 禁止
catch (Exception e)吞异常:Controller 中不得使用try-catch(Exception)包裹所有逻辑。应依赖全局异常处理器(GlobalExceptionHandler)统一处理
water 项目现状:
AppController几乎每个方法都被try { ... } catch (Exception e) { return fail(e); }包裹。这会:
- 吞掉异常栈,排查问题困难
- 与框架全局异常处理器功能重复
- 代码冗余严重
正确做法:移除 try-catch,让异常自然抛出由全局处理器捕获。仅在需要业务降级或资源清理时才 catch
- catch 块不得为空:
catch (Exception e) {}是严重 bug。至少要记录日志 - 不得 catch 后丢弃异常信息:
catch (Exception e) { log.error("出错"); }丢失了堆栈,应使用log.error("出错", e)
🟡 Suggestion
- 异常分类处理:业务异常用
ServiceException/UserException,系统异常让其传播 - 自定义异常携带上下文:抛出异常时包含设备编号、用户 ID 等关键信息
1.5 安全规范
🔴 Blocker
- SQL 注入防护:所有数据库查询必须使用 MyBatis Plus 的
LambdaQueryWrapper或参数化查询,禁止字符串拼接 SQL - 所有接口必须有权限控制:使用
@SaCheckLogin/@SaCheckPermission/@SaCheckRole或在方法内校验用户所有权
water 项目正面案例:
AppController中assertDeviceOwned(deviceNo)校验设备归属权,这个模式是对的
- 敏感数据不得出现在日志中:密码、token、手机号完整信息等不得记录到日志
- TLS 证书校验不得全局关闭:
TrustAllManager等信任所有证书的代码必须有明确的安全边界注释,且仅限内网测试环境
water 项目现状:
MqttClientManager.createTrustAllSslContext()创建了信任所有证书的 TrustManager。如果用于生产环境是 🔴 Blocker
- 输入校验:所有外部输入(HTTP 参数、MQTT 消息体)必须做格式校验和长度限制
1.6 性能规范
🔴 Blocker
- 禁止 N+1 查询:在循环中查询数据库/Redis 必须改为批量查询
water 项目现状:
MqttCommandAckService.findPendingCommandsByDeviceNo()遍历所有 pending commandId 逐个查 Redis,当 pending 命令多时性能差。应改用 Redis 的MGET或 Hash 结构批量获取
AppController.scheduleDeviceList()在 for 循环中逐个设备调用appSchedulingDeviceService.findByDeviceNo(),是典型的 N+1 查询
🟡 Suggestion
- 批量操作优先:
deleteDevice中的for (deviceNo) { assertDeviceOwned(deviceNo); }应改为批量查询 - 避免在热路径创建重对象:
new SimpleDateFormat()应替换为DateTimeFormatter(线程安全且无需重复创建)
water 项目现状:
AppController.switchDevice()中new SimpleDateFormat("yyyy-MM-dd HH:mm:ss").format(new Date())每次请求都创建新实例。应改为:private static final DateTimeFormatter DATE_TIME_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss"); // 使用 String startTime = LocalDateTime.now().format(DATE_TIME_FORMATTER);
- Redis Key 设置 TTL:所有写入 Redis 的缓存必须设置过期时间,防止内存泄漏
1.7 代码整洁规范
🟡 Suggestion
- 删除注释掉的代码:被注释的代码应删除,版本历史由 Git 管理。
// @SaCheckRole("appadmin")这类应清理 - 魔法值提取为常量或枚举:状态值
"0"/"1"应定义为枚举
water 项目正面案例:
MqttCommandAckService中的CommandLockResult枚举是好的实践。命令状态"0"/"1"也应类似处理
- 重复代码提取:相同逻辑出现 3 次以上必须提取为公共方法
water 项目现状:
DeviceCommandServiceImpl.parseStartTime()有 95 行,包含三段几乎相同的 try-catch 块解析不同时间格式,应提取为通用方法
💭 Nit
- 方法行数不超过 50 行:超过的考虑拆分
- 方法参数不超过 5 个:超过的考虑封装为参数对象
- import 不得使用通配符:
import java.util.*应写明具体类
1.8 并发规范
🔴 Blocker
- 共享可变状态必须加锁:多线程访问的可变字段必须使用同步机制保护
- 锁必须设置超时:
tryLock()必须带超时参数,禁止无限等待导致死锁
water 项目正面案例:
MqttCommandAckService.withCommandLock()使用 Redisson 分布式锁并设置了超时,这是正确的做法
- SimpleDateFormat 非线程安全:禁止在多线程环境共享
SimpleDateFormat实例
🟡 Suggestion
- 优先使用不可变对象:能声明
final的就声明final - 优先使用并发集合:
ConcurrentHashMap代替HashMap+ synchronized
1.9 测试规范
🟡 Suggestion
- 核心业务逻辑必须有单元测试:Service 层的公共方法和 MQTT Handler 必须有测试覆盖
- 测试命名规范:
methodName_scenario_expectedResult,如handleAck_validPayload_ackConfirmed - 测试必须独立:不依赖执行顺序,不依赖外部状态
water 项目正面案例:
MqttCommandAckService的静态方法ackMatchesPendingCommand和resolveMissingCommandId被设计为可测试的纯函数,这是好的实践
1.10 日志规范
🔴 Blocker
- 日志必须包含上下文:MQTT 相关日志必须包含设备编号、命令编号等关键信息
water 项目正面案例:
log.warn("[MQTT] 收到空 ACK 设备编号={}", deviceNo)包含了设备编号,格式统一
🟡 Suggestion
- 日志级别正确使用:
ERROR用于系统异常、WARN用于业务异常、INFO用于关键业务流程、DEBUG用于调试信息 - 使用占位符而非字符串拼接:
log.info("设备{}上线", deviceNo)而非log.info("设备" + deviceNo + "上线")
2. 代码审查流程
2.1 角色定义
| 角色 | 职责 | 资质要求 |
|---|---|---|
| 提交者 (Author) | 编写代码、自测、提交 PR、响应审查意见 | 熟悉项目编码规范 |
| 审查者 (Reviewer) | 审查代码质量、提出改进意见、确认修复 | 熟悉相关模块业务逻辑 |
| 合并者 (Merger) | 最终确认、合并代码 | 技术负责人或模块 Owner |
2.2 PR 提交前(自检阶段)
提交者在创建 PR 前必须完成:
- 本地编译通过:
mvn clean compile -DskipTests - 单元测试通过:
mvn test(不得使用-DskipTests) - 静态分析通过:Checkstyle + SpotBugs 无 Error 级别问题
- 自检 Checklist:逐项核对 Section 3 检查清单
- PR 描述:包含变更说明、测试方式、影响范围
PR 描述模板
## 变更说明
<!-- 简述本次改了什么、为什么改 -->
## 测试方式
<!-- 如何验证本次变更 -->
## 影响范围
<!-- 影响哪些模块/功能 -->
## 关联 Issue
<!-- Closes #xxx -->
2.3 审查阶段
审查时间要求
| PR 规模 | 审查时限 | 说明 |
|---|---|---|
| 小型 (< 100 行) | 4 小时内 | 单个 bugfix 或小功能 |
| 中型 (100-500 行) | 1 个工作日内 | 常规功能开发 |
| 大型 (> 500 行) | 2 个工作日内 | 大功能或重构,建议拆分 |
审查步骤
Step 1: 全局审视
├── PR 描述是否清晰?
├── 变更范围是否合理?
└── 是否有对应的测试?
Step 2: 逐文件审查
├── 按 [Section 3 检查清单] 逐项核对
├── 标注问题等级 (🔴/🟡/💭)
└── 给出具体的修改建议和原因
Step 3: 运行验证
├── 代码能否编译通过?
├── 测试是否通过?
└── 静态分析是否有新问题?
Step 4: 总结
├── Approve — 无 Blocker,可合并
├── Request Changes — 有 Blocker 或多个 Suggestion
└── Comment — 仅有 Nit 或讨论性问题
2.4 合并标准
PR 必须满足以下全部条件才能合并:
- 零 🔴 Blocker:所有 Blocker 已修复
- Suggestion 有明确处理:已修复或标注为延期(需技术负责人确认)
- 至少 1 个 Approve:中型以上 PR 需要模块 Owner Approve
- CI 通过:编译、测试、静态分析全部通过
- 无未解决的讨论:所有 review comment 已 resolved
2.5 冲突升级机制
| 冲突类型 | 升级路径 |
|---|---|
| 审查意见分歧 | 提交者与审查者协商 → 协商不成由模块 Owner 裁决 |
| 架构方案分歧 | 模块 Owner → 技术负责人(你)最终裁决 |
| 紧急修复绕过审查 | 需技术负责人批准,事后 24 小时内补审查 |
2.6 紧急修复流程
生产环境紧急 bug 修复可走快速通道:
- 技术负责人批准走紧急流程
- 最少 1 人审查(可简化为只查 Blocker 级别问题)
- 合并后 24 小时内补完整审查
- 记录在
docs/hotfix-log.md中
3. 代码审查检查清单
审查者逐项核对,每项标记 ✅ 通过 / ❌ 未通过 / ➖ 不适用
3.1 安全 (Security)
| # | 检查项 | 等级 |
|---|---|---|
| S1 | 所有数据库查询使用参数化查询,无 SQL 注入风险 | 🔴 |
| S2 | 所有接口有权限控制(@SaCheckLogin / 所有权校验) |
🔴 |
| S3 | 敏感数据(密码、token)不出现在日志中 | 🔴 |
| S4 | 外部输入(HTTP 参数、MQTT 消息)做了格式校验和长度限制 | 🔴 |
| S5 | TLS 证书校验未在生产环境关闭 | 🔴 |
| S6 | 文件上传做了类型和大小限制 | 🟡 |
3.2 正确性 (Correctness)
| # | 检查项 | 等级 |
|---|---|---|
| C1 | 代码逻辑是否实现了预期功能 | 🔴 |
| C2 | 边界条件是否处理(空值、空集合、零值、最大值) | 🔴 |
| C3 | 异常路径是否正确处理(不吞异常、不丢失异常栈) | 🔴 |
| C4 | 并发场景下数据一致性是否保证 | 🔴 |
| C5 | Redis Key 都设置了 TTL | 🟡 |
| C6 | 分布式锁设置了超时时间 | 🟡 |
3.3 架构 (Architecture)
| # | 检查项 | 等级 |
|---|---|---|
| A1 | Controller 不含业务逻辑(仅参数接收+调用 Service) | 🔴 |
| A2 | 无跨层调用(Controller 不直接调 Mapper) | 🔴 |
| A3 | 无循环依赖(未使用 @Lazy 掩盖) |
🔴 |
| A4 | 单个类行数不超过 500 行 | 🟡 |
| A5 | Service 有接口定义 | 🟡 |
| A6 | 使用构造器注入(@RequiredArgsConstructor) |
🟡 |
3.4 代码质量 (Code Quality)
| # | 检查项 | 等级 |
|---|---|---|
| Q1 | 集合使用泛型,无 Raw Type | 🔴 |
| Q2 | 无魔法值(字符串/数字硬编码),常量已提取 | 🟡 |
| Q3 | 无注释掉的代码 | 🟡 |
| Q4 | 重复代码已提取为公共方法 | 🟡 |
| Q5 | 变量命名有意义,无单字母变量 | 🟡 |
| Q6 | 方法行数不超过 50 行 | 💭 |
| Q7 | import 无通配符 | 💭 |
3.5 性能 (Performance)
| # | 检查项 | 等级 |
|---|---|---|
| P1 | 无 N+1 查询(循环内不查数据库/Redis) | 🔴 |
| P2 | 批量操作使用批量接口 | 🟡 |
| P3 | 热路径无重对象创建(如 SimpleDateFormat) |
🟡 |
| P4 | 大集合操作考虑分页或流式处理 | 🟡 |
3.6 异常处理 (Error Handling)
| # | 检查项 | 等级 |
|---|---|---|
| E1 | 无 catch (Exception e) 包裹全部逻辑 |
🔴 |
| E2 | catch 块不为空,至少记录日志 | 🔴 |
| E3 | 日志使用 log.error("msg", e) 保留异常栈 |
🔴 |
| E4 | 业务异常用 ServiceException,不混用 |
🟡 |
3.7 测试 (Testing)
| # | 检查项 | 等级 |
|---|---|---|
| T1 | 核心业务逻辑有单元测试 | 🟡 |
| T2 | 测试覆盖正常路径和异常路径 | 🟡 |
| T3 | 测试命名规范,能表达意图 | 💭 |
| T4 | 测试独立运行,不依赖顺序 | 🟡 |
3.8 日志 (Logging)
| # | 检查项 | 等级 |
|---|---|---|
| L1 | 关键操作有日志记录 | 🟡 |
| L2 | 日志包含上下文信息(设备号、用户ID等) | 🟡 |
| L3 | 日志级别使用正确 | 💭 |
| L4 | 使用占位符而非字符串拼接 | 💭 |
4. 静态分析工具集成方案
4.1 工具选型
| 工具 | 作用 | 集成方式 | 优先级 |
|---|---|---|---|
| Checkstyle | 代码风格检查(命名、import、行数) | Maven 插件 | P0 立即集成 |
| SpotBugs | 潜在 bug 检测(空指针、资源泄漏) | Maven 插件 | P0 立即集成 |
| SonarQube | 综合质量平台(重复率、覆盖率、复杂度) | 独立服务 + Scanner | P1 二期集成 |
4.2 Maven 集成配置
在根 pom.xml 的 <build><plugins> 中添加以下配置:
<!-- Checkstyle: 代码风格检查 -->
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<version>3.5.0</version>
<dependencies>
<dependency>
<groupId>com.puppycrawl.tools</groupId>
<artifactId>checkstyle</artifactId>
<version>10.18.0</version>
</dependency>
</dependencies>
<configuration>
<configLocation>checkstyle.xml</configLocation>
<consoleOutput>true</consoleOutput>
<failsOnError>true</failsOnError>
<includeTestSourceDirectory>true</includeTestSourceDirectory>
</configuration>
<executions>
<execution>
<id>validate</id>
<phase>validate</phase>
<goals>
<goal>check</goal>
</goals>
</execution>
</executions>
</plugin>
<!-- SpotBugs: 静态 bug 分析 -->
<plugin>
<groupId>com.github.spotbugs</groupId>
<artifactId>spotbugs-maven-plugin</artifactId>
<version>4.8.6.4</version>
<dependencies>
<dependency>
<groupId>com.github.spotbugs</groupId>
<artifactId>spotbugs</artifactId>
<version>4.8.6</version>
</dependency>
</dependencies>
<configuration>
<effort>Max</effort>
<threshold>Low</threshold>
<failOnError>true</failOnError>
<excludeFilterFile>spotbugs-exclude.xml</excludeFilterFile>
</configuration>
<executions>
<execution>
<id>spotbugs-check</id>
<phase>verify</phase>
<goals>
<goal>check</goal>
</goals>
</execution>
</executions>
</plugin>
4.3 实施路线
| 阶段 | 周期 | 目标 |
|---|---|---|
| Phase 1 | 第 1 周 | 集成 Checkstyle,配置规则文件,CI 中运行 |
| Phase 2 | 第 2 周 | 集成 SpotBugs,修复现有 High 级别问题 |
| Phase 3 | 第 3-4 周 | 搭建 SonarQube 服务,建立质量基线 |
| Phase 4 | 持续 | 将静态分析纳入 PR 检查,设置质量门禁 |
4.4 过渡策略
重要:集成静态分析工具时,现有代码会有大量违规。采取以下策略:
- 新代码零容忍:PR 中新增/修改的代码必须通过检查
- 存量代码分期修复:用 SonarQube 的 "New Code" 模式,只关注新增问题
- 基线快照:记录当前问题数量作为基线,要求只减不增
5. 附录:water 项目典型问题案例
案例一:上帝控制器 + 异常吞噬
文件: AppController.java
问题等级: 🔴 Blocker (架构) + 🔴 Blocker (异常处理)
// ❌ 当前代码 — 940 行上帝类,每个方法 try-catch 包裹
@PostMapping("/bindDeviceStatus")
public R<Map> bindDeviceStatus(@RequestBody String body) {
try {
AppDeviceBo appDevice = JsonUtils.parseObject(body, AppDeviceBo.class);
appDevice.setUserId(LoginHelper.getUserId());
Map<String, Object> map = appDeviceService.bindDeviceStatus(appDevice);
return R.ok(map);
} catch (Exception e) {
return fail(e); // 吞异常栈,与全局处理器重复
}
}
// ✅ 改进后 — 移到独立 Controller,移除 try-catch
@RestController
@RequestMapping("/app/v1/device")
@RequiredArgsConstructor
public class AppDeviceCommandController extends BaseController {
private final IAppDeviceService appDeviceService;
@ApiEncrypt
@PostMapping("/bindDeviceStatus")
public R<Map<String, Object>> bindDeviceStatus(@RequestBody String body) {
AppDeviceBo appDevice = JsonUtils.parseObject(body, AppDeviceBo.class);
appDevice.setUserId(LoginHelper.getUserId());
return R.ok(appDeviceService.bindDeviceStatus(appDevice));
}
}
案例二:Raw Type + 魔法值
文件: AppController.java
问题等级: 🔴 Blocker (Raw Type) + 🟡 Suggestion (魔法值)
// ❌ 当前代码
Map retMap = new HashMap(); // Raw Type
List l = new ArrayList<>(); // 单字母变量 + 推断丢失泛型
String status = map.get("workStatus"); // 魔法字符串
ack.setStatus("1"); // 魔法值
// ✅ 改进后
Map<String, Object> resultMap = new HashMap<>();
List<AppDeviceVo> availableDevices = new ArrayList<>();
String status = params.get(DeviceCommand.FIELD_WORK_STATUS);
ack.setStatus(CommandStatus.SUCCESS.getCode()); // 枚举
案例三:N+1 查询
文件: AppController.java — scheduleDeviceList() 方法
问题等级: 🔴 Blocker (性能)
// ❌ 当前代码 — 循环内逐个查询
for (AppDeviceVo appDeviceVo : appDeviceVos) {
List<AppSchedulingDeviceVo> scheduleDevices =
appSchedulingDeviceService.findByDeviceNo(appDeviceVo.getDeviceNo()); // N+1!
if (scheduleDevices.size() == 0) {
appDeviceVoList.add(appDeviceVo);
}
}
// ✅ 改进后 — 批量查询
List<String> deviceNos = appDeviceVos.stream()
.map(AppDeviceVo::getDeviceNo)
.collect(Collectors.toList());
Set<String> scheduledDeviceNos = appSchedulingDeviceService
.findScheduledDeviceNos(deviceNos); // 一次批量查询
List<AppDeviceVo> availableDevices = appDeviceVos.stream()
.filter(d -> !scheduledDeviceNos.contains(d.getDeviceNo()))
.collect(Collectors.toList());
案例四:线程安全问题
文件: AppController.java — switchDevice() 方法
问题等级: 🔴 Blocker (线程安全)
// ❌ 当前代码 — SimpleDateFormat 非线程安全
String startTime = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss").format(new Date());
// ✅ 改进后 — 使用 DateTimeFormatter(线程安全)
private static final DateTimeFormatter DATE_TIME_FORMATTER =
DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss");
String startTime = LocalDateTime.now().format(DATE_TIME_FORMATTER);
案例五:正面案例 — MQTT 消息分发策略模式
文件: MqttMessageDispatcher.java + MqttTopicHandler.java
评价: 🟢 优秀设计
// 策略接口
public interface MqttTopicHandler {
String getTopicPattern();
void handle(String topic, String payload);
}
// 分发器 — 新增 Topic 处理器零改动
// 这是开闭原则的标准实践,值得团队学习
值得推广的模式:
- 策略模式解耦消息分发
- 有界队列 + 多消费者的高并发架构
- 优雅停机设计(
DisposableBean+awaitTermination) - 分布式锁保护并发操作
6. 推广与落地建议
6.1 分阶段实施
| 阶段 | 时间 | 目标 | 负责人 |
|---|---|---|---|
| 宣贯 | 第 1 周 | 团队学习本文档,理解审查标准 | 技术负责人 |
| 工具 | 第 2 周 | 集成 Checkstyle + SpotBugs,修复 P0 问题 | 全员 |
| 试运行 | 第 3-4 周 | 所有 PR 执行审查流程,以学习为主 | 全员 |
| 正式执行 | 第 5 周起 | 严格执行 Blocker 零容忍 | 全员 |
6.2 度量指标
| 指标 | 目标 | 度量方式 |
|---|---|---|
| PR 审查覆盖率 | 100% | 所有合并的 PR 都经过审查 |
| Blocker 修复率 | 100% | 所有 Blocker 在合并前修复 |
| 平均审查响应时间 | < 1 工作日 | PR 提交到首次审查 |
| 静态分析问题数 | 逐月递减 | SonarQube 趋势图 |
| 单元测试覆盖率 | 核心模块 > 60% | JaCoCo 报告 |
6.3 持续改进
- 每月复盘:审查中发现的共性问题汇总,更新本标准
- 季度培训:针对高频问题组织技术分享
- 标准迭代:本文档每季度评审一次,根据团队反馈调整
文档维护:技术负责人 | 最后更新:2026-07-16