★ FEATURED ARTICLE 老炮踩坑录 · V04 ·老炮视野系列基于「企业融合评估平台」真实源码224 个 Java 文件、35256 行、25 个 Mapper XML20 条清单条条有反例关键词代码审查 · Review 顺序 · grep 五分钟 · 真实反例 欢迎阅读个人主页知守观我的专栏老炮踩坑录当前内容代码阅读文章目录前言第一遍grep 五分钟——机器能查的别用眼睛第二遍看结构——不读逻辑也能查的部分第三遍读语义——眼睛该花的地方第四遍问一句生产上会不会出问题完整速查表老炮点评下期预告前言这个专栏到现在为止我已写了二十几篇后台有位读者问了我一个问题“你文章里那些坑是 review 的时候一眼看出来的还是事后踩了才知道的”诚实的说两者都有。但一眼看出来的部分靠的不是眼力是顺序。我 review 代码有个固定顺序四遍。前两遍基本不动脑靠的是命令行扫数字、眼睛看结构真正值钱的是第三遍。拿这个 2022 年的老项目当靶子我把整个顺序跑一遍每个环节抽出清单凑齐 20 条每条后面都附上项目里的真实反例。你可以把这篇当成一次 review 的现场直播。第一遍grep 五分钟——机器能查的别用眼睛很多人 review 打开文件就从第一行往下读读到哪算哪。我的习惯是先让机器跑一圈。这几条没有一条需要读代码。第 1 条printStackTrace 还有多少。grep -r printStackTrace src/ | wc -l → 117117 处分布在 26 个文件里。光 HttpUtil 一个工具类就占了 27 处。printStackTrace 的问题讲过不止一次异常打到 stdout标准输出生产环境没有堆栈落盘出问题时只剩一句不知道为啥失败了。我的通过标准很简单核心业务代码里这个数字应该是 0。第 2 条System.out 有多少。→ 88 处原始统计刨掉注释里的和我自己写的两个复现类正式代码里还有几十处散在 18 个文件中这里说句实话grep 出来的数字是带水分的StringUtils.java 里那 22 处大部分是在注释和 javadoc 里。所以这条的用法是看趋势数字过百就要警惕具体几处得点开看。第 3 条空 catch 块。14 处。最典型的是 SSL.java 里的这段catch(Exceptionex){// 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 行StringappKey105p23ffdiopafb822ed7201;正式代码里其实用了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。它自己还带着一个复制粘贴的 bugNOT_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。StringapprovalState(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一个查不到任何数据的占位常量用户选了某些组合就会看到空列表还不会报错。有个细节我没完全确认这20条清单在不同项目类型上优先级会不会不一样比如业务系统和技术中间件review的重点应该不同。我本地跑的是业务系统中间件项目没试过有踩过坑的兄弟评论区指点一下。第四遍问一句生产上会不会出问题第四遍只问一个问题这段代码在并发、断网、数据量翻十倍的情况下表现是什么。第 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 也没指定编码中文内容过这条链编码全看服务器脸色。完整速查表#检查项一条命令 / 一眼特征本项目实况我的通过标准1printStackTracegrep 计数117 处 / 26 文件核心业务为 02System.outgrep 计数几十处 / 18 文件正式代码为 03空 catchgrep 多行匹配14 处0注释不算处理4catch 里只打印就返回catch 里只有 printStackTrace return15 处必须记日志或重抛5Executors 快捷工厂Executors.前缀3 处一律打回6Mapper 里的 ${}grep\${活 6 处 注释 2 处每个都能说清可信来源7文件行数按行数排序9 个 800 行超 500 行先拆再谈8数字命名方法doPost2、doPost3HttpUtil 五连出现即打回9重复代码同一逻辑多份签名 URL 三份第三处必须收编10测试代码进主干main 方法 / 小写类名testurl.java011密钥域名硬编码密钥、IP、域名出现在 .javaappKey 明文等012TODO 处理grep TODO仅 5 处存疑有人清才算数13状态机唯一来源状态定义有几套注释/枚举/常量/裸串四套全项目一处定义14枚举可用性枚举有无调用方0 调用且带 bug有人用且写得对15裸字符串状态码equals(“0”) 链大量走常量/枚举16equals 防 NPE变量.equals(常量)多处常量在前17嵌套层级if 套 if 超三层四层嵌套 300 行超三层换写法18ThreadLocal 成对set/remove 配对remove 零次finally 里 remove19事务/异步的异常路径Transactionalcatch、Async 裸奔均有推演完整异常路径20资源关闭finally 手工 close7 个 if 关三层try-with-resources自查打卡搜一下你的项目grep -r “catch (Exception” src/ | wc -l看看有多少个catch块。打在评论区我看看谁的异常处理最“密集”。老炮点评这张清单的顺序是有讲究的能用 grep 的不用眼睛能用眼睛的不过脑子脑子只花在语义和推演上。很多人 review 慢慢在顺序反了——上来就逐行读读两小时漏掉的还是那些一条命令就能扫出来的东西。还有一层清单里每一条后面挂的反例都是这个项目真实付过代价或者正在埋着的隐患。我 review 代码有个习惯打回的时候必须附证据——文件、行号、会出什么问题。只说这么写不好对方多半不服也不知道改到什么程度算完把证据摆出来对方自己能得出同样的结论。新人可能会忘了一个知识点但这条上次出过事他能记住下次自己就会绕开。如果你还没看过这个项目的代码腐化问题可以翻一下《代码腐化的五个信号》最后问一句你 review 代码时是先 grep 扫一遍还是直接打开文件从头读评论区聊聊我看看有多少人跟我一样“先让机器跑一圈”。下期预告《做了 18 年开发我总结的十条军规》这张 20 条的清单讲的是怎么看别人的代码下期换个方向如果只能给团队立十条规矩我立哪十条。每条军规背后还是这个项目的一个翻车现场——包括几个今天没来得及讲的。如果本文对你有帮助欢迎 点赞 ⭐ 收藏 关注 留言你的每一次互动都是我继续更新的动力我们下一篇见我是老炮18 年 Java 老兵仍在一线。关注「Java老炮踩坑录」不错过每一篇真实案例少踩坑。
阅读完成 · 觉得有帮助？
