老炮踩坑录 · V04 · 老炮视野系列
· 基于「企业融合评估平台」真实源码：224 个 Java 文件、35256 行、25 个 Mapper XML，20 条清单条条有反例
· 关键词：代码审查 · Review 顺序 · grep 五分钟 · 真实反例
👋 欢迎阅读
🏠个人主页：知守观
📘我的专栏： 老炮踩坑录
💻当前内容：代码审查
前言
这个专栏到现在为止我已写了二十几篇，后台有位读者问了我一个问题："你文章里那些坑，是 review 的时候一眼看出来的，还是事后踩了才知道的？"
诚实的说两者都有。但"一眼看出来"的部分，靠的不是眼力，是顺序。
我 review 代码有个固定顺序，四遍。前两遍基本不动脑，靠的是命令行扫数字、眼睛看结构；真正值钱的是第三遍。拿这个 2022 年的老项目当靶子，我把整个顺序跑一遍，每个环节抽出清单，凑齐 20 条，每条后面都附上项目里的真实反例。
你可以把这篇当成一次 review 的现场直播。
第一遍：grep 五分钟——机器能查的，别用眼睛
很多人 review 打开文件就从第一行往下读，读到哪算哪。我的习惯是先让机器跑一圈。这几条没有一条需要读代码。
第 1 条：printStackTrace 还有多少。
grep -r "printStackTrace" src/ | wc -l
→ 117
117 处，分布在 26 个文件里。光 HttpUtil 一个工具类就占了 27 处。printStackTrace 的问题讲过不止一次：异常打到 stdout标准输出，生产环境没有堆栈落盘，出问题时只剩一句"不知道为啥失败了"。我的通过标准很简单：核心业务代码里这个数字应该是 0。
第 2 条：System.out 有多少。
→ 88 处（原始统计），刨掉注释里的和我自己写的两个复现类，正式代码里还有几十处，散在 18 个文件中
这里说句实话：grep 出来的数字是带水分的，StringUtils.java 里那 22 处大部分是在注释和 javadoc 里。所以这条的用法是"看趋势"，数字过百就要警惕，具体几处得点开看。
第 3 条：空 catch 块。
14 处。最典型的是 SSL.java 里的这段：
catch (Exception ex) {
// Ignore
}
SSL 证书配置失败，静默忽略，然后 HTTPS 请求在别的地方抛出一个八竿子打不着的错。// Ignore 这三个字符，是写代码的人给查问题的人挖的坑。
第 4 条：catch 里只有 printStackTrace 然后 return。
15 处，FileController 一个文件占 10 处。这比空 catch 更隐蔽——看起来"处理了"，实际把异常上下文全丢了，调用方拿到一个正常返回，以为一切顺利。 @Transactional 事务失效排查 讲过的"事务没回滚"就是这种写法养出来的。
第 5 条：Executors 快捷工厂。
3 处。newFixedThreadPool(5) 的无界队列问题在 Executors 线程池无界队列隐患审计 里专门写过，这里只说一句：看到 Executors. 前缀就直接打回，没有例外。
第 6 条：Mapper XML 里的 ${}。
grep -r "\${" mapping/
# 活的6处，另有2处躺在注释掉的SQL里
这条我要多说两句，因为它最能说明 review 不是模式匹配。AND ati.approvalState IN (${approvalState}) 看着像标准的 SQL 注入写法，我顺着调用链追了一遍：Controller 里 params.getString("approvalStateCity") 拿的是前端参数，但中间过了 ApprovalStateUtil 的白名单分支，能进 ${} 的只有写死的常量。也就是说，外部参数目前到不了这个 ${} 里，注入打不进来。
但 Controller 里还留着一段注释掉的旧代码，当年是 data.put("approvalState", params.getString(...)) 裸传直通。也就是说这个 ${} 曾经真的能注入，后来靠加白名单才拦住，但写法还留在那。下一个写新查询的人如果绕过白名单直接传参，注入风险就回来了。${} 在 mapper 里出现一次，我就要追一次完整链路——这一条通过的标准是"每个 ${} 都能说清来源且来源可信"。
第二遍：看结构——不读逻辑，也能查的部分
第一遍跑完，这个项目代码质量大概什么水平，我心里有数了。第二遍看结构，依然不怎么需要动脑子。
第 7 条：文件行数排行榜。
按行数排序，前九名全部超过 800 行：ApplyInfoServiceImpl 1796 行、EnterpriseRegistController 1600行、ApplyElecInfoServiceImpl 1488 行……800 行以上的文件有 9 个。Java Controller 写了1600行怎么办 和 Java 代码腐化的五个信号 把这个说透了，这里只留个标准：单文件过 500 行，review 时我会要求拆完再谈。
第 8 条：名字里带数字的方法。
HttpUtil.java 里有 doPost、doPost2、doPost3、doPost4、doPost5。五个方法，八百多行。方法名带数字只有一个含义：上一个方法不敢改，复制了一份改吧改吧。每次看到这种命名，我就知道这个类已经没人敢动了。
第 9 条：同一段代码有几份。
问卷星的签名 URL 拼接逻辑，我在三个地方找到了它：testurl.java、WjxUtils.java、ApplyElecInfoServiceImpl.java 第 752 行。三份的参数处理还略有差异。哪天签名算法要改，改几处？谁能想全？重复代码的通过标准：同样的逻辑抄到第三处时，必须收编。
第 10 条：测试代码混进 src/main。
testurl.java，类名小写开头，里面一个 main 方法，作用是打印一条问卷星 URL。这种"本地调试完忘了删"的文件进了主干，还跟着打包发布。顺手说一句，它里面还有明文 appKey。
第 11 条：密钥和域名写死。
testurl.java 第 25 行：
String appKey = "105p23ffdiopafb822ed7201";
正式代码里其实用了 @Value("${wjx.appKey}") 从配置读，但这个测试文件把真 key 留进了代码库。HttpUtil 里还有写死的 Referer: http://login.xiaomayi.xuyi。密钥、域名、IP，这三样出现在 .java 文件里，一律打回。
第 12 条：TODO 的数量。
全项目 TODO/FIXME 只有 5 处。这个数字我拿不准该怎么解读——可能是项目健康，也可能是压根没人标 TODO。从这个项目其他问题的表现看，我倾向于后者。TODO 少从来不值得表扬，值得表扬的是 TODO 有人清。
第三遍：读语义——眼睛该花的地方
前两遍二十分钟就能跑完，省下的时间全用在这一遍。语义问题没有捷径，只能读。
第 13 条：状态机有几个版本。
这个项目的审核状态，我找到四个"权威来源"：代码注释里写了一套（9 个状态），ApprovalStateEnum 枚举里定义了一套（6 个状态），SysContants 里常量都定义了两套（int 一套、String 一套），业务代码里实际用的却是裸字符串 "0"、"-5"、"1"。四套互不完全对得上。
第 14 条：枚举有没有人用，枚举自己对不对。
ApprovalStateEnum 这个枚举，全项目调用次数：0。它自己还带着一个复制粘贴的 bug：
NOT_SELECT("-2", "遴选未通过") //
, SELECTED("3", "遴选未通过") // 3 应该是"遴选通过"
code 3 的 label 从上一行抄下来忘了改。另外 valueByCode 写成了实例方法——想按 code 查枚举，得先有一个枚举实例。这种"设计了但没法用"的枚举，比没有枚举更糟，它给读代码的人制造"这里已经有规范了"的错觉。
第 15 条：状态码是不是裸字符串满天飞。
BackDeclareListServiceImpl 里一长串 approvalState.equals("0")、approvalState.equals("-5")。讽刺的是同项目里 SysContants 定义了常量，ApprovalStateUtil 里大部分地方也用了常量，但第 118 行突然冒出一个 tjState.append("2,")——同一个方法里，常量和裸字符串混着用。规范做了一半，比没做更容易误导人。
第 16 条：equals 前面那个东西会不会是 null。
String approvalState = (String)e.get("approvalState");
if(!approvalState.equals("0") && !approvalState.equals("-5")){
从 Map 里取出来直接 .equals，数据库里这列要是有 NULL，这里就会抛 NPE。老规矩反着写："0".equals(approvalState)，或者用 Objects.equals。这条我看 diff 时会逐行扫，已经习惯了。
第 17 条：一个方法里嵌套了几层。
ApprovalStateUtil.queryState，三百来行，四层 if-else 嵌套，干的活是"把三个下拉框的选项翻译成一串状态码"。同样的状态码注释在文件里抄了五遍。这个需求一张映射表就能表达清楚，嵌套 if 每加一层，漏一个分支的概率就翻一倍——事实上它已经漏了：好几个分支 append 的是 APPROVAL_STATE_NO，一个"查不到任何数据"的占位常量，用户选了某些组合就会看到空列表，还不会报错。
第四遍：问一句"生产上会不会出问题"
第四遍只问一个问题：这段代码在并发、断网、数据量翻十倍的情况下，表现是什么。
第 18 条：ThreadLocal 的 set 和 remove 成不成对。
全项目 set 了，remove 零次。ThreadLocal + @Async 导致用户数据串号 那篇"用户数据串了"就是这么来的，不展开。
第 19 条：@Transactional 和 catch 有没有同框，@Async 有没有配线程池。
前者是 @Transactional 事务失效排查 的"吞掉异常事务没回滚"，后者是 ThreadLocal + @Async 导致用户数据串号 里"靠 SimpleAsyncTaskExecutor 不池化赌运气"。这两个注解同框出现 catch 或者裸奔出现时，我会把整段方法的异常路径推演一遍。
第 20 条：资源怎么关。
doPost 的 finally 里躺着 7 个 if：先关 br、再关 rsd、再关 is，装饰器三层关三遍，关的过程自己还可能抛异常，于是又套了一层 try-catch-printStackTrace。这个项目跑在 Java 8 上，try-with-resources 一次都没出现。顺手提一句，OutputStreamWriter 也没指定编码，中文内容过这条链，编码全看服务器脸色。
完整速查表
| # | 检查项 | 一条命令 / 一眼特征 | 本项目实况 | 我的通过标准 |
|---|---|---|---|---|
| 1 | printStackTrace | grep 计数 | 117 处 / 26 文件 | 核心业务为 0 |
| 2 | System.out | grep 计数 | 几十处 / 18 文件 | 正式代码为 0 |
| 3 | 空 catch | grep 多行匹配 | 14 处 | 0，注释不算处理 |
| 4 | catch 里只打印就返回 | catch 里只有 printStackTrace + return | 15 处 | 必须记日志或重抛 |
| 5 | Executors 快捷工厂 | Executors. 前缀 | 3 处 | 一律打回 |
| 6 | Mapper 里的 ${} | grep \${ | 活 6 处 + 注释 2 处 | 每个都能说清可信来源 |
| 7 | 文件行数 | 按行数排序 | 9 个 800+ 行 | 超 500 行先拆再谈 |
| 8 | 数字命名方法 | doPost2、doPost3 | HttpUtil 五连 | 出现即打回 |
| 9 | 重复代码 | 同一逻辑多份 | 签名 URL 三份 | 第三处必须收编 |
| 10 | 测试代码进主干 | main 方法 / 小写类名 | testurl.java | 0 |
| 11 | 密钥域名硬编码 | 密钥、IP、域名出现在 .java | appKey 明文等 | 0 |
| 12 | TODO 处理 | grep TODO | 仅 5 处，存疑 | 有人清才算数 |
| 13 | 状态机唯一来源 | 状态定义有几套 | 注释/枚举/常量/裸串四套 | 全项目一处定义 |
| 14 | 枚举可用性 | 枚举有无调用方 | 0 调用且带 bug | 有人用且写得对 |
| 15 | 裸字符串状态码 | equals("0") 链 | 大量 | 走常量/枚举 |
| 16 | equals 防 NPE | 变量.equals(常量) | 多处 | 常量在前 |
| 17 | 嵌套层级 | if 套 if 超三层 | 四层嵌套 300 行 | 超三层换写法 |
| 18 | ThreadLocal 成对 | set/remove 配对 | remove 零次 | finally 里 remove |
| 19 | 事务/异步的异常路径 | @Transactional+catch、@Async 裸奔 | 均有 | 推演完整异常路径 |
| 20 | 资源关闭 | finally 手工 close | 7 个 if 关三层 | try-with-resources |
老炮点评
这张清单的顺序是有讲究的：能用 grep 的不用眼睛，能用眼睛的不过脑子，脑子只花在语义和推演上。
很多人 review 慢，慢在顺序反了——上来就逐行读，读两小时，漏掉的还是那些一条命令就能扫出来的东西。
还有一层：清单里每一条后面挂的反例，都是这个项目真实付过代价或者正在埋着的隐患。我 review 代码有个习惯，打回的时候必须附证据——文件、行号、会出什么问题。只说"这么写不好"，对方多半不服，也不知道改到什么程度算完；把证据摆出来，对方自己能得出同样的结论。新人可能会忘了一个知识点，但"这条上次出过事"他能记住，下次自己就会绕开。
如果你还没看过这个项目的代码腐化问题，可以翻一下《代码腐化的五个信号》
下期预告
《做了 18 年开发，我总结的十条军规》
这张 20 条的清单讲的是"怎么看别人的代码"，下期换个方向：如果只能给团队立十条规矩，我立哪十条。每条军规背后，还是这个项目的一个翻车现场——包括几个今天没来得及讲的。
如果本文对你有帮助，欢迎：
👍 点赞 ｜ ⭐ 收藏 ｜ 👤 关注 ｜ 💬 留言
你的每一次互动都是我继续更新的动力，我们下一篇见！🚀
我是老炮，18 年 Java 老兵，仍在一线。关注「Java老炮踩坑录」，不错过每一篇真实案例，少踩坑。
