633 lines
24 KiB
Markdown
633 lines
24 KiB
Markdown
# Water 项目代码审查标准与流程
|
||
|
||
> **版本**: 1.0 | **适用项目**: water (IoT 智能灌溉设备管理系统) | **技术栈**: Spring Boot 3.5 + Java 17 + MyBatis Plus + MQTT + Redis
|
||
|
||
---
|
||
|
||
## 目录
|
||
|
||
1. [代码审查标准](#1-代码审查标准)
|
||
2. [代码审查流程](#2-代码审查流程)
|
||
3. [代码审查检查清单](#3-代码审查检查清单)
|
||
4. [静态分析工具集成方案](#4-静态分析工具集成方案)
|
||
5. [附录：water 项目典型问题案例](#5-附录water-项目典型问题案例)
|
||
|
||
---
|
||
|
||
## 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); }` 包裹。这会：
|
||
> 1. 吞掉异常栈，排查问题困难
|
||
> 2. 与框架全局异常处理器功能重复
|
||
> 3. 代码冗余严重
|
||
>
|
||
> **正确做法**：移除 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())` 每次请求都创建新实例。应改为：
|
||
> ```java
|
||
> 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 前必须完成：
|
||
|
||
1. **本地编译通过**：`mvn clean compile -DskipTests`
|
||
2. **单元测试通过**：`mvn test`（不得使用 `-DskipTests`）
|
||
3. **静态分析通过**：Checkstyle + SpotBugs 无 Error 级别问题
|
||
4. **自检 Checklist**：逐项核对 [Section 3 检查清单](#3-代码审查检查清单)
|
||
5. **PR 描述**：包含变更说明、测试方式、影响范围
|
||
|
||
#### PR 描述模板
|
||
|
||
```markdown
|
||
## 变更说明
|
||
<!-- 简述本次改了什么、为什么改 -->
|
||
|
||
## 测试方式
|
||
<!-- 如何验证本次变更 -->
|
||
|
||
## 影响范围
|
||
<!-- 影响哪些模块/功能 -->
|
||
|
||
## 关联 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. 技术负责人批准走紧急流程
|
||
2. 最少 1 人审查（可简化为只查 Blocker 级别问题）
|
||
3. 合并后 24 小时内补完整审查
|
||
4. 记录在 `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>` 中添加以下配置：
|
||
|
||
```xml
|
||
<!-- 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 过渡策略
|
||
|
||
> **重要**：集成静态分析工具时，现有代码会有大量违规。采取以下策略：
|
||
|
||
1. **新代码零容忍**：PR 中新增/修改的代码必须通过检查
|
||
2. **存量代码分期修复**：用 SonarQube 的 "New Code" 模式，只关注新增问题
|
||
3. **基线快照**：记录当前问题数量作为基线，要求只减不增
|
||
|
||
---
|
||
|
||
## 5. 附录：water 项目典型问题案例
|
||
|
||
### 案例一：上帝控制器 + 异常吞噬
|
||
|
||
**文件**: `AppController.java`
|
||
**问题等级**: 🔴 Blocker (架构) + 🔴 Blocker (异常处理)
|
||
|
||
```java
|
||
// ❌ 当前代码 — 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); // 吞异常栈，与全局处理器重复
|
||
}
|
||
}
|
||
```
|
||
|
||
```java
|
||
// ✅ 改进后 — 移到独立 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 (魔法值)
|
||
|
||
```java
|
||
// ❌ 当前代码
|
||
Map retMap = new HashMap(); // Raw Type
|
||
List l = new ArrayList<>(); // 单字母变量 + 推断丢失泛型
|
||
String status = map.get("workStatus"); // 魔法字符串
|
||
ack.setStatus("1"); // 魔法值
|
||
```
|
||
|
||
```java
|
||
// ✅ 改进后
|
||
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 (性能)
|
||
|
||
```java
|
||
// ❌ 当前代码 — 循环内逐个查询
|
||
for (AppDeviceVo appDeviceVo : appDeviceVos) {
|
||
List<AppSchedulingDeviceVo> scheduleDevices =
|
||
appSchedulingDeviceService.findByDeviceNo(appDeviceVo.getDeviceNo()); // N+1!
|
||
if (scheduleDevices.size() == 0) {
|
||
appDeviceVoList.add(appDeviceVo);
|
||
}
|
||
}
|
||
```
|
||
|
||
```java
|
||
// ✅ 改进后 — 批量查询
|
||
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 (线程安全)
|
||
|
||
```java
|
||
// ❌ 当前代码 — SimpleDateFormat 非线程安全
|
||
String startTime = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss").format(new Date());
|
||
```
|
||
|
||
```java
|
||
// ✅ 改进后 — 使用 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`
|
||
**评价**: 🟢 优秀设计
|
||
|
||
```java
|
||
// 策略接口
|
||
public interface MqttTopicHandler {
|
||
String getTopicPattern();
|
||
void handle(String topic, String payload);
|
||
}
|
||
|
||
// 分发器 — 新增 Topic 处理器零改动
|
||
// 这是开闭原则的标准实践，值得团队学习
|
||
```
|
||
|
||
**值得推广的模式**：
|
||
1. 策略模式解耦消息分发
|
||
2. 有界队列 + 多消费者的高并发架构
|
||
3. 优雅停机设计（`DisposableBean` + `awaitTermination`）
|
||
4. 分布式锁保护并发操作
|
||
|
||
---
|
||
|
||
## 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*
|