etms/CODE_REVIEW_GUIDE.md

871 lines
23 KiB
Markdown
Raw Permalink Normal View History

# JCDP 项目代码审查标准与流程规范
> **文档版本**v1.0 | **创建时间**2026-04-16 | **适用项目**ETMS 教育培训管理系统JCDP
>
> **审查范围**Java 源码、SQL 映射文件(*.map.xml、FreeMarker 模板(*.ftl、JavaScript*.js、CSS*.css
---
## 目录
1. [审查目标与原则](#1-审查目标与原则)
2. [审查角色与职责](#2-审查角色与职责)
3. [代码质量等级定义](#3-代码质量等级定义)
4. [SQL 映射文件审查标准](#4-sql-映射文件审查标准)
5. [FreeMarker 模板审查标准](#5-freemarker-模板审查标准)
6. [JavaScript 审查标准](#6-javascript-审查标准)
7. [CSS 审查标准](#7-css-审查标准)
8. [安全审查标准](#8-安全审查标准)
9. [审查流程规范](#9-审查流程规范)
10. [审查检查清单](#10-审查检查清单)
11. [常见问题与修复建议](#11-常见问题与修复建议)
---
## 1. 审查目标与原则
### 1.1 审查目标
| 目标 | 说明 |
|------|------|
| **提升代码质量** | 发现并修复潜在缺陷,降低线上故障率 |
| **统一代码风格** | 保持项目内代码风格一致,降低维护成本 |
| **知识传承** | 通过审查促进团队技术交流,提升整体水平 |
| **风险控制** | 识别安全漏洞、性能瓶颈、数据风险 |
### 1.2 审查原则
1. **建设性优先**:审查是教导,不是批评。每条反馈都应教会开发者一些东西
2. **抓大放小**:阻塞性问题必须修复,风格问题酌情处理
3. **自动化辅助**:能自动化的检查(如格式、风格)不占用人工审查时间
4. **具体可操作**:每条审查意见都应明确指出位置、问题和建议
---
## 2. 审查角色与职责
### 2.1 角色定义
| 角色 | 职责 | 适用场景 |
|------|------|----------|
| **作者 (Author)** | 提交代码、完成自检、响应审查意见 | 所有场景 |
| **审查者 (Reviewer)** | 执行代码审查、给出反馈、批准/驳回 | 所有场景 |
| **架构师/技术负责人** | 审批重大变更、制定技术规范、处理争议 | 高风险变更 |
| **质量门禁 (Gatekeeper)** | 验证审查通过、检查清单执行 | 合入主分支前 |
### 2.2 审查者分配原则
| 变更类型 | 最低审查人数 | 审查者要求 |
|----------|-------------|------------|
| **常规功能** | 1 人 | 任意团队成员 |
| **业务逻辑变更** | 1-2 人 | 至少 1 人为同模块开发者 |
| **核心模块变更** | 2 人+ | 至少 1 人为架构师/技术负责人 |
| **安全/权限相关** | 2 人+ | 必须包含安全专员或架构师 |
---
## 3. 代码质量等级定义
### 3.1 问题严重等级
| 等级 | 标记 | 描述 | 处理要求 |
|------|------|------|----------|
| **阻塞** | 🔴 | 必须修复才能合入 | 修复前禁止合入 |
| **严重** | 🟠 | 强烈建议修复 | 修复后再审查 |
| **建议** | 🟡 | 应该修复 | 可以在后续迭代修复 |
| **挑剔** | 💭 | 可选优化 | 不阻塞合入 |
### 3.2 问题类型分类
| 类型 | 说明 | 示例 |
|------|------|------|
| **Bug** | 功能性错误 | 逻辑错误、空指针风险 |
| **安全** | 安全漏洞 | SQL 注入、XSS、权限绕过 |
| **性能** | 性能问题 | N+1 查询、大数据处理 |
| **可维护性** | 代码可读性 | 命名不清晰、重复代码 |
| **风格** | 代码风格 | 格式不规范、无注释 |
---
## 4. SQL 映射文件审查标准
### 4.1 🔴 阻塞问题
#### 4.1.1 SQL 注入风险
**问题描述**:用户输入直接拼接到 SQL 语句中。
**典型问题代码**
```xml
<!-- 危险写法 -->
<@p p="AND name = %s" f="?">name</@p> <!-- 使用 ? 绑定参数 -->
<@p p="AND name LIKE '%' + ? + '%'">keyword</@p> <!-- LIKE 模糊查询 -->
```
**正确写法**
```xml
<!-- 推荐LIKE 模糊查询使用 CONCAT -->
<@p p="AND name LIKE CONCAT('%', ?, '%')">keyword</@p>
<!-- 推荐IN 查询使用参数化 -->
<@p p="AND id IN(%s)" f="?">id</@p> <!-- id 应为逗号分隔的字符串 -->
```
**检查要点**
- [ ] 所有用户输入是否通过 `?` 参数绑定
- [ ] LIKE 模糊查询是否使用 `CONCAT` 函数
- [ ] IN 查询的列表参数是否正确处理
#### 4.1.2 缺少 WHERE 条件
**问题描述**UPDATE/DELETE 语句缺少 WHERE 条件,可能导致全表操作。
**典型问题代码**
```xml
<!-- 危险写法 -->
<sql id="deleteByIds">
DELETE FROM table_name WHERE id IN(<@p f="?">id</@p>)
</sql>
<!-- 如果 id 为空或 null可能导致全表删除 -->
```
**正确写法**
```xml
<!-- 推荐:先判断非空 -->
<sql id="deleteByIds">
DELETE FROM table_name
WHERE 1=1
<@p p="AND id IN(%s)" f="?">id</@p>
</sql>
<!-- 或在业务层先校验 id 非空 -->
```
**检查要点**
- [ ] UPDATE/DELETE 语句必须有 WHERE 条件
- [ ] WHERE 条件中的动态参数是否为必填项
- [ ] 是否存在 `1=1` 而后没有实际条件的情况
#### 4.1.3 字段大小写混用
**问题描述**:同一文件中字段名大小写不统一,增加维护难度。
**典型问题代码**
```xml
<!-- 问题ID 大小写混用 -->
FROM JCDP_SYS_USER USR
LEFT JOIN JCDP_SYS_USER_EXT EXT ON USR.ID = EXT.ID
WHERE USR.ID = ? <!-- 有时大写有时小写 -->
```
**正确写法**
```xml
<!-- 统一使用大写或驼峰 -->
FROM JCDP_SYS_USER USR
LEFT JOIN JCDP_SYS_USER_EXT EXT ON USR.id = EXT.id
WHERE USR.id = ?
```
### 4.2 🟠 严重问题
#### 4.2.1 废弃代码未清理
**问题描述**:存在"备份"、"以前"、"old"等注释标记的废弃 SQL。
**典型问题代码**
```xml
<!-- ❌ 发现的问题代码 -->
<sql id="getOutTrainDbList以前的写法备份">
select ot.* from et_train_ot ot,et_train_ot_xy xy where ...
</sql>
```
**修复建议**
- 删除所有废弃 SQL 定义
- 如需保留历史版本,使用版本控制系统
**检查要点**
- [ ] 是否存在 `backup`、`备份`、`old`、`以前` 等注释
- [ ] 是否有重复的 SQL ID 定义
#### 4.2.2 子查询嵌套过深
**问题描述**:三层及以上的子查询影响性能和可读性。
**典型问题代码**
```xml
<!-- 问题:三层嵌套 -->
SELECT * FROM A
WHERE id IN (
SELECT id FROM B
WHERE id IN (
SELECT id FROM C WHERE ...
)
)
```
**修复建议**
- 拆分为多个简单查询
- 使用 JOIN 替代子查询
- 考虑在应用层处理
#### 4.2.3 全表扫描风险
**问题描述**LIKE 查询使用前导通配符导致索引失效。
**典型问题代码**
```xml
<@p p="AND name LIKE '%' + ? + '%'">keyword</@p>
```
**修复建议**
- 考虑使用 Elasticsearch 等全文搜索引擎
- 或限制 LIKE 查询的使用场景
### 4.3 🟡 建议问题
#### 4.3.1 SQL 命名不规范
**问题描述**SQL ID 命名不符合规范。
**规范命名**
| 操作类型 | 命名示例 |
|----------|----------|
| 查询单条 | `getXxxById` |
| 查询列表 | `getXxxList`、`listXxx` |
| 插入 | `insertXxx`、`saveXxx` |
| 更新 | `updateXxx` |
| 删除 | `deleteXxx`、`removeXxx` |
| 统计 | `countXxx`、`getXxxCount` |
**检查要点**
- [ ] SQL ID 是否符合命名规范
- [ ] 是否按功能模块分组
#### 4.3.2 SELECT * 使用
**问题描述**:使用 `SELECT *` 返回不必要字段。
**建议**
```xml
<!-- 不推荐 -->
SELECT * FROM table_name
<!-- 推荐:明确指定字段 -->
SELECT id, name, status FROM table_name
```
**例外情况**
- 快速原型开发
- JOIN 后的复杂查询(字段过多时)
#### 4.3.3 缺少分页
**问题描述**:列表查询未限制返回数量。
**建议**
```xml
<!-- 推荐:添加分页参数 -->
<sql id="getXxxList">
SELECT * FROM table_name WHERE 1=1
<@p p="AND (%s)" s=" OR " f="%S LIKE ?">keyword</@p>
<@p p="LIMIT %d,%d">pageSize,offset</@p>
</sql>
```
### 4.4 💭 挑剔问题
- ORDER BY 字段未建立索引
- 未使用 LIMIT 限制结果集大小
- 日期比较未使用函数(如使用字符串比较日期)
---
## 5. FreeMarker 模板审查标准
### 5.1 🔴 阻塞问题
#### 5.1.1 XSS 跨站脚本风险
**问题描述**:用户输入未转义直接输出到 HTML。
**典型问题代码**
```html
<!-- 危险写法:直接输出用户输入 -->
<span>${userName}</span>
<div>{{userName}}</div> <!-- JS 模板未转义 -->
```
**正确写法**
```html
<!-- FreeMarker使用 ${userName?html} 转义 -->
<span>${userName?html}</span>
<!-- JS 模板:使用双花括号自动转义或手动处理 -->
<span>{{value.name}}</span>
```
**检查要点**
- [ ] 所有用户输入是否经过转义
- [ ] 富文本内容是否使用专门的 XSS 过滤
- [ ] JS 中拼接 HTML 是否使用 text() 而非 html()
#### 5.1.2 内联事件处理
**问题描述**:在 HTML 中使用内联事件处理器。
**典型问题代码**
```html
<!-- 不推荐 -->
<button onclick="delete({{value.id}})" />
<input onchange="submit(this.value)" />
```
**正确写法**
```html
<!-- 推荐:使用 data 属性 + JS 事件绑定 -->
<button class="btn-delete" data-id="{{value.id}}">删除</button>
<script>
$('.btn-delete').on('click', function() {
var id = $(this).data('id');
// ...
});
</script>
```
#### 5.1.3 注释掉的死代码
**问题描述**:存在大量被注释的代码,影响可维护性。
**典型问题代码**
```html
<#-- 2019年第一次项目管理培训考试试卷1-->
<#-- 项目管理培训是对管理者...-->
<span id="title_name"></span>
```
**修复建议**
- 删除所有注释掉的代码
- 使用版本控制系统保留历史
### 5.2 🟠 严重问题
#### 5.2.1 内联样式过多
**问题描述**:大量使用 `style=""` 属性。
**典型问题代码**
```html
<div style="height: 86px; margin-left: 20px; width: 96%;">
```
**修复建议**
```html
<!-- 使用 class 引用 CSS -->
<div class="form-title">
```
**检查要点**
- [ ] 是否存在超过 3 个 style 属性的元素
- [ ] 重复的样式定义是否提取为 CSS 类
#### 5.2.2 条件嵌套过深
**典型问题代码**
```html
{{if value.type=="单选题"}}
{{if value.optiona.length}}
{{if value.user_answer=="A"}}
<div>...</div>
{{/if}}
{{/if}}
{{/if}}
```
**修复建议**
```html
<!-- 使用逻辑运算符 -->
{{if value.type=="单选题" && value.optiona.length && value.user_answer=="A"}}
<div>...</div>
{{/if}}
<!-- 或使用 helper 函数处理复杂逻辑 -->
{{isChecked(value, "A")}}
```
#### 5.2.3 重复代码模式
**典型问题代码**
```html
<!-- 单选题和多选题的选项渲染代码几乎相同 -->
{{if value.user_answer=="A"}}<div>...A选项...</div>{{/if}}
{{if value.user_answer=="B"}}<div>...B选项...</div>{{/if}}
<!-- 多选题中重复以上逻辑 -->
```
**修复建议**
- 提取为公共宏(`<#macro>`
- 使用 JS 渲染选项
### 5.3 🟡 建议问题
#### 5.3.1 表单验证缺失
**检查要点**
- [ ] 表单是否有必填项校验
- [ ] 输入格式是否校验(如日期、数字)
- [ ] 提交前是否有确认提示
#### 5.3.2 错误处理缺失
**检查要点**
- [ ] AJAX 请求是否有错误处理
- [ ] 失败时是否有用户提示
- [ ] 网络异常是否友好提示
#### 5.3.3 加载状态缺失
**检查要点**
- [ ] 异步操作是否有 loading 状态
- [ ] 数据加载中是否有骨架屏或占位
### 5.4 💭 挑剔问题
- 按钮文字未使用 i18n
- 图标使用图片而非字体图标
- 响应式布局缺失
---
## 6. JavaScript 审查标准
### 6.1 🔴 阻塞问题
#### 6.1.1 XSS 风险
**典型问题代码**
```javascript
// 危险:直接插入用户输入到 DOM
element.innerHTML = userInput;
$('#container').html(userInput);
```
**正确写法**
```javascript
// 安全:使用 text() 或手动转义
element.textContent = userInput;
$('#container').text(userInput);
// 如果必须插入 HTML先进行 XSS 过滤
$('#container').html(sanitize(userInput));
```
#### 6.1.2 未声明的全局变量
**典型问题代码**
```javascript
// 全局污染
function doSomething() {
result = process(); // 缺少 var/let/const
}
```
**正确写法**
```javascript
function doSomething() {
const result = process();
}
```
**检查要点**
- [ ] 是否使用 `'use strict'`
- [ ] 变量是否使用 var/let/const 声明
- [ ] 是否存在命名冲突
### 6.2 🟠 严重问题
#### 6.2.1 变量声明错误
**典型问题代码**
```javascript
// 缺少 var
isIDCard1 = /^[1-9]\d{7}.../;
isPositiveNumber = /^[1-9]\d*\.\d*.../;
```
**正确写法**
```javascript
const isIDCard1 = /^[1-9]\d{7}((0\d)|(1[0-2]))(([0|1|2]\d)|3[0-1])\d{3}$/;
const isPositiveNumber = /^[1-9]\d*\.\d*$|^0\.\d*[1-9]\d*$|^[1-9]\d*$/;
```
#### 6.2.2 日期 API 误用
**典型问题代码**
```javascript
// 错误getDay() 返回星期几(0-6)getDate() 返回几号(1-31)
if (dateTime1.getDay() == dateTime2.getDay()) {
```
**正确写法**
```javascript
if (dateTime1.getDate() == dateTime2.getDate()) {
```
#### 6.2.3 同步 AJAX 请求
**典型问题代码**
```javascript
$.ajax({
type: 'post',
async: false, // 同步请求,阻塞 UI
url: dictBaseUrl + dictKey,
// ...
});
```
**修复建议**
- 改为异步请求
- 使用 Promise/async-await
- 考虑数据预加载
### 6.3 🟡 建议问题
#### 6.3.1 代码重复
**检查要点**
- [ ] 相似的逻辑是否提取为函数
- [ ] 是否存在复制粘贴的重复代码
#### 6.3.2 魔法数字
**典型问题代码**
```javascript
if (dateTime1.getDay() - dateTime2.getDay() == 1) {
tempTime = dateTime2.format("昨天 hh:mm");
}
```
**修复建议**
```javascript
const YESTERDAY = 1;
if (dateTime1.getDay() - dateTime2.getDay() == YESTERDAY) {
tempTime = dateTime2.format("昨天 hh:mm");
}
```
#### 6.3.3 缺少 JSDoc 注释
**建议**:核心函数添加 JSDoc 注释
```javascript
/**
* 获取当前日期
* @param {string} [dateSp='-'] - 日期分隔符
* @returns {string} 格式化日期,如 "2026-04-16"
*/
function getDateNow(dateSp) {
dateSp = dateSp || '-';
// ...
}
```
### 6.4 💭 挑剔问题
- console.log 未在生产环境移除
- 未使用 ES6+ 语法
- 未使用 lint 工具
---
## 7. CSS 审查标准
### 7.1 🟡 建议问题
#### 7.1.1 选择器嵌套过深
**典型问题代码**
```css
.container .content .main .sidebar .box .title {
font-size: 14px;
}
```
**修复建议**
```css
.box-title { /* 使用语义化的类名 */
font-size: 14px;
}
```
#### 7.1.2 未使用 CSS 变量
**建议**:使用 CSS 变量统一管理主题色等
```css
:root {
--primary-color: #1890ff;
--text-color: #333;
}
.button {
background: var(--primary-color);
color: var(--text-color);
}
```
#### 7.1.3 未使用 BEM 命名
**建议**:使用 BEM 命名规范
```css
/* Block */
.train-card { }
/* Element */
.train-card__title { }
.train-card__content { }
/* Modifier */
.train-card--disabled { }
```
---
## 8. 安全审查标准
### 8.1 🔴 阻塞安全问题
#### 8.1.1 SQL 注入
| 检查点 | 说明 |
|--------|------|
| 用户输入是否参数化 | 所有用户输入必须通过 `?` 绑定 |
| LIKE 查询是否安全 | 使用 `CONCAT('%', ?, '%')` |
| IN 查询是否安全 | 确保列表参数处理正确 |
#### 8.1.2 XSS 跨站脚本
| 检查点 | 说明 |
|--------|------|
| 用户输入是否转义 | 使用 `?html``sanitize()` |
| 富文本是否过滤 | 使用专门的 XSS 过滤库 |
| URL 参数是否编码 | 使用 `encodeURIComponent()` |
#### 8.1.3 权限绕过
| 检查点 | 说明 |
|--------|------|
| 前端权限检查是否重复后端 | 前后端双重校验 |
| 越权访问是否防护 | 资源归属校验 |
| 敏感操作是否验证 | 删除/审批等操作二次确认 |
#### 8.1.4 敏感信息泄露
| 检查点 | 说明 |
|--------|------|
| 日志是否打印敏感信息 | 密码、Token 等脱敏 |
| 错误信息是否泄露 | 避免暴露堆栈信息 |
| 配置文件是否安全 | 数据库密码等加密存储 |
### 8.2 🟠 严重安全问题
#### 8.2.1 CSRF 跨站请求伪造
**检查点**
- [ ] 关键操作是否使用 CSRF Token
- [ ] Token 是否正确验证
#### 8.2.2 文件上传漏洞
**检查点**
- [ ] 文件类型是否验证
- [ ] 文件内容是否检查
- [ ] 上传路径是否隔离
---
## 9. 审查流程规范
### 9.1 审查流程图
```
┌─────────────────────────────────────────────────────────────────┐
│ 代码提交流程 │
└─────────────────────────────────────────────────────────────────┘
┌─────────────────────────────────────────────────────────────────┐
│ 1. 开发完成 → 开发者自检 │
│ - 运行本地检查清单 │
│ - 确保单元测试通过(未来) │
│ - 代码格式化 │
└─────────────────────────────────────────────────────────────────┘
┌─────────────────────────────────────────────────────────────────┐
│ 2. 提交 Pull Request / Merge Request │
│ - 填写 PR 描述(改了什么、为什么改、影响范围) │
│ - 关联需求/缺陷单 │
│ - 指定审查者 │
└─────────────────────────────────────────────────────────────────┘
┌─────────────────────────────────────────────────────────────────┐
│ 3. 审查者审查 │
│ - 全面审查代码 │
│ - 给出反馈(阻塞/建议/问题) │
│ - 标注审查通过/需修改 │
└─────────────────────────────────────────────────────────────────┘
┌───────────┴───────────┐
│ │
▼ ▼
┌──────────────┐ ┌──────────────┐
│ 通过 │ │ 需修改 │
│ │ │ │
│ 合入主分支 │ │ 返回开发者 │
└──────────────┘ │ 修改后重新 │
│ 提交审查 │
└──────────────┘
```
### 9.2 审查时间要求
| 变更类型 | 审查响应时间 | 总体处理时间 |
|----------|-------------|-------------|
| 紧急修复Hotfix | 1 小时内 | 4 小时内 |
| 常规功能 | 24 小时内 | 3 个工作日内 |
| 大型变更 | 48 小时内 | 1 周内 |
### 9.3 审查通过标准
#### 必须满足的条件
1. ✅ 所有 🔴 阻塞问题已修复
2. ✅ 所有 🟠 严重问题已修复或已有明确修复计划
3. ✅ 审查者明确批准(至少 1 人)
4. ✅ 分支合并前无冲突
#### 鼓励但不强制
- 🟡 建议问题全部修复
- 💭 挑剔问题尽量修复
---
## 10. 审查检查清单
### 10.1 提交前自检清单(开发者)
#### SQL 映射文件
- [ ] 所有用户输入使用参数绑定 `?`
- [ ] LIKE 查询使用 `CONCAT` 函数
- [ ] 无废弃/备份代码
- [ ] 无重复的 SQL ID
- [ ] 字段名大小写一致
- [ ] UPDATE/DELETE 有 WHERE 条件
#### FreeMarker 模板
- [ ] 用户输入已转义
- [ ] 无注释掉的死代码
- [ ] 无内联事件处理
- [ ] 无过多内联样式
- [ ] 表单有验证
#### JavaScript
- [ ] 无全局污染
- [ ] 使用 var/let/const 声明
- [ ] AJAX 使用异步
- [ ] 错误有处理
- [ ] 无同步阻塞
#### 通用
- [ ] 代码格式统一
- [ ] 无硬编码配置
- [ ] 关键逻辑有注释
### 10.2 审查清单(审查者)
#### 功能正确性
- [ ] 代码逻辑正确
- [ ] 边界条件处理
- [ ] 错误处理完整
- [ ] 兼容性考虑
#### 安全
- [ ] 无 SQL 注入
- [ ] 无 XSS 漏洞
- [ ] 权限校验完整
- [ ] 敏感信息保护
#### 性能
- [ ] 无 N+1 查询
- [ ] 无大对象处理
- [ ] 无阻塞主线程
#### 可维护性
- [ ] 命名清晰
- [ ] 无重复代码
- [ ] 注释充分
- [ ] 逻辑简洁
---
## 11. 常见问题与修复建议
### 11.1 SQL 映射常见问题
| 问题 | 原因 | 修复建议 |
|------|------|----------|
| 参数绑定失败 | 使用了 `%s` 而非 `?` | 检查动态 SQL 语法 |
| LIKE 查询失效 | 直接拼接 `%` | 使用 `CONCAT('%', ?, '%')` |
| 全表操作 | WHERE 条件为空 | 添加必填参数校验 |
### 11.2 FreeMarker 常见问题
| 问题 | 原因 | 修复建议 |
|------|------|----------|
| XSS 风险 | 直接输出用户输入 | 使用 `?html` 转义 |
| 页面错乱 | 内联样式冲突 | 提取为 CSS 类 |
| 逻辑复杂 | 嵌套条件过多 | 使用 helper 函数 |
### 11.3 JavaScript 常见问题
| 问题 | 原因 | 修复建议 |
|------|------|----------|
| 变量未定义 | 缺少声明 | 使用 var/let/const |
| 日期错误 | getDay/getDate 混淆 | 使用 getDate() |
| 界面卡顿 | 同步 AJAX | 改为异步 |
---
## 附录
### A. 参考资料
- [Google JavaScript Style Guide](https://google.github.io/styleguide/jsguide.html)
- [Alibaba Java Coding Guidelines](https://alibaba.github.io/Alibaba-Java-Coding-Guidelines/)
- [OWASP Top 10](https://owasp.org/www-project-top-ten/)
### B. 工具推荐
| 工具 | 用途 |
|------|------|
| ESLint | JavaScript 代码检查 |
| SonarQube | 代码质量分析 |
| Checkstyle | Java 代码检查 |
| SQLFluff | SQL 语法检查 |
### C. 修订记录
| 版本 | 日期 | 修改人 | 修改内容 |
|------|------|--------|----------|
| v1.0 | 2026-04-16 | AI | 初始版本 |
---
*本文档由 AI 代码审查专家制定,适用于 JCDP/ETMS 项目。请根据项目实际情况调整。*