代码评审如何做好

最新推荐文章于 2025-06-15 22:16:16 发布

ManFresh

最新推荐文章于 2025-06-15 22:16:16 发布

阅读量1.9k

点赞数 2

CC 4.0 BY-SA版权

分类专栏：产品、市场和管理

本文链接：https://blog.youkuaiyun.com/zhang_yin_liang/article/details/113876208

产品、市场和管理专栏收录该内容

18 篇文章

订阅专栏

一、背景知识

1.1、关于

Code Review 翻译成中文是代码评审，具体的定义可以看 wiki。这篇 wiki 介绍说 Code Review 在帮助团队找到代码缺陷这件事上作用巨大：“代码审查一般可以找到及移除约65%的错误，最高可以到85%”。实际上， Code Review 的好处远不止这一条，它至少能在以下三个方面帮到我们：

传播知识。相信很多人第一次提交 Code Review 都有类似的经历：短短几百行代码，却被提了密密麻麻几十条 comments，更新了十多次代码，才最终被 accept 。其实当代码被 accept，提交代码的工程师通过这次 review 就学习到了代码规范和很多好的实践。同时，通过 review 更资深工程师的代码，年轻的工程师也更直观地学习架构和编码；另外，工程师之间也可以通过 review 代码来共享项目知识，看代码实现在绝大多数时候是了解项目的最好方式。
增进代码质量。这点也很容易理解，有经验的工程师可以在架构设计、代码细节等各个方面帮助到初学者。不同工程师也会有知识盲点，互相 review 进步也很快。另外，被 review 的代码质量更高还有一个很多人注意不到的心理因素：在状态不佳的时候，工程师难免会匆忙写些“潦草”的代码，但是当你知道自己的代码会被review 的工程师提交 comment 打回来，自然会更仔细些 : -)
找出潜在的 bug。将损失降至最低。这是大部分团队进行 Code Review 的目的。就像上面提到的，Code Review 在这方面效果不错。其实我认为大部分代码 bug 应该由单元测试，功能测试，性能测试和回归测试来保障。不过由于静态分析不理解业务，另外有些 bug 在测试中并不容易复现，这两种情况下，经验丰富的工程师来 review 代码就尤其重要了。

1.2、我是如何审代码的

1、首先我会关注代码风格规范是否和公司一致（可读性和可维护性），

2、然后关注代码调用是否破坏了当前代码架构，

3、其次是代码逻辑中的资源处理、线程、安全、异常处理，

4、再其次是业务实现逻辑，

5、最后是否有优化空间。

前面三点相对于参考checklist能够做到，部分处理也能够通过工具检查查出来，后面两点相对比较难，需要了解它的需求是什么样，需要了解这块的逻辑，然后才能够读懂逻辑实现是否有误。最后基于对于好的代码理解看看在扩展性和性能上是否有优化空间。

二、什么叫做好代码评审

那么我们怎样做好 Code Review 呢？两个方面：一是减负，Code Review 只做它应该做的事情。二是提高 Code Review 的效率。

2.1、如何提高效率

首先, 先列几个不该在Code Review时关注的

Code review 不应该承担发现代码错误的职责。Code Review主要是审核代码的质量，如可读性，可维护性，以及程序的逻辑和对需求和设计的实现。代码中的bug和错误应该由单元测试，功能测试，性能测试，回归测试来保证的（其中主要是单元测试，因为那是最接近Bug，也是Bug没有扩散的地方）
Code review 不应该成为保证代码风格和编码标准的手段。编码风格和代码规范都属于死的东西，每个程序员在把自己的代码提交团队Review的时候，代码就应该是符合规范的，这是默认值，属于每个人自己的事情，不应该交由团队来完成，否则只会浪费大家本来就不够的时间。

注意: 这里说在Code Review时不该关注, 并不是说不重要, 而是说, 这些部分应该有其他方式(例如自动化工具)去保障, 因为人的成本是最高的, 机器和程序才是便宜的。

Code Review 应该讨论的是功能实现、架构设计、代码质量。

1、检查代码风格和编程规范。

代码风格包括但不限于：命名规范、代码缩进、注释和文档等等

2、检查常规的 bad smell 和代码 bug

重复代码、过长函数等等

2.2、如何提高 Code Review 的效率

选用合适的工具。我用过 Phabricator、Gerrit、Gitlab 来 review 代码。这里推荐使用 Phabricator，就不过多介绍了，可以看这里的讨论。
每次只 Review 少量代码。新人经常犯的一个错误，花了两周甚至更多的时间写代码，然后又花了一些时间做测试，等他们自己觉得“大功告成”才敢自信地提交 review，殊不知，这个时候提交的 review 几乎没有什么意义。一方面，对于提交 review 的工程师来说，辛辛苦苦开发测试了两三周，就等 review 完成后 release 了，这时候如果收到各种需要修改代码的意见，心里难免会有抵触情绪，尤其是 review 的结果是架构需要改动，代码需要大面积重写，内心一定是崩溃的。另一方面，代码审查者如果面对数千甚至上万行代码，需要理清项目架构都需要花费大量时间，强行 review 这种代码，争论、修改、测试代码，很可能一周甚至更长时间都完成不了 review，结果就是双方都痛苦不堪。在实践过程中，我们认为，频繁 review 代码并且每次只 review 少量代码非常重要，我自己认为 reivew 不超过 500 行代码比较好。
明确职责。 Review 过程中经常遇到的一个问题是 review 响应不及时。比如 assgin 给了工程师，工程师没有及时 Review，或者提交 review 的工程师没有及时响应修改意见。造成这种现象的原因大致有这么几种：工程师没有及时处理 review 的习惯；review 工程师需要项目的领域知识等。解决方法也很简单，Review 是项目开发的一部分，进度由开发工程师来负责，这样，Code Review 如果不顺利，应该由提交代码的工程师负责推动，如果需要讲解代码，提交代码的工程师应该主动走到 reviewer 工位上，说说背景知识和代码逻辑。也就是说，如果沟通受阻，工程师应该更积极的面对 reviewer ，毕竟大家是在花自己的时间来帮助他。
整理 checklist。如果你回顾犯过的编程错误就会发现，在某个阶段自己容易犯类似的错误。其实上，团队里的工程师也有这种情况。刚入门的工程师会出现 API 误用；刚加入团队的工程师对编码规范需要一段时间来适应；有些工程师在代码命名上总会犯同样的错误；也有一些工程师搞不清楚并发编程；还有工程师甚至常常写面条式的代码而不自知。如果每位工程师有自己的 checklist 来记录这些问题，定期回顾自己是不是还在犯类似的错误，他们的水平就会很快提高，至少不容易重复已经犯过的错误。同样，团队也需要积累 Code Review 的 checklist，刚开始，这个 list 可能非常初级，会有一些常见的 bad small，甚至代码规范的内容。不过没关系，相信随着团队成员能力的提升，这个 list 慢慢会集中在设计方面，比如编写代码的工程师有没有考虑到代码可维护性、扩展性和性能等等。
完善CI/CD设施。很多团队不愿意做 Code Review，其实和不愿意做单元测试、集成测试原因一样，这些团队的CI/CD 工具不成熟，每增加一个不直接产出“功能代码”的步骤就会增加工作负担。其实根本原因是工程效率工具的缺失。
培养工程师的能力。还有一个比较常见问题是，有些新人面对被 review 代码往往提不出问题。这个时候就需要工程师提升自己的能力。如果平时有积累，面对等待 review 的代码，即使不能面面俱到，也能提供不少有价值的意见，比如整理学习 Restful API 知识，在评审 Http 接口代码时就会是专家；掌握了 Spring 框架，Guava 等工具类，也能在很多时候提出很好的建议。慢慢地积累更多经验后，这些工程师就能提出更多业务、设计方面的意见。

2.3、Code Review 的实际操作建议

代码审查是应该在互相沟通中进行讨论的，而不是相互对抗。预先确定哪些是要点哪些不是，可以减少冲突并拟定预期。
经常进行Code Review, 不要攒了1w行才让同事帮你review, 这是坑队友.
- 要Review的代码越多，那么要重构，重写的代码就会越多。而越不被程序作者接受的建议也会越多，唾沫口水战也会越多。
- 程序员代码写得时候越长，程序员就会在代码中加入越来越多的个人的东西。程序员最大的问题就是“自负”，无论什么时候，什么情况下，有太多的机会会让这种“自负”澎涨开来，并开始影响团队影响整个项目，以至于听不见别人的建议，从而让Code Review变成了口水战。
- 越接近软件发布的最终期限，代码也就不能改得太多。
Code Review不要太正式，而且要短
尽可能的让不同的人Reivew你的代码(不要超过3个人)
- 从不同的方向评审代码总是好的。
- 会有更多的人帮你在日后维护你的代码。
- 这也是一个增加团队凝聚力的方法。
保持积极的正面的态度

无论是代码作者，还是评审者，都需要一种积极向上的正面的态度，作者需要能够虚心接受别人的建议，因为别人的建议是为了让你做得更好；评审者也需要以一种积极的正面的态度向作者提意见，因为那是和你在一个战壕里的战友。记住，你不是一段代码，你是一个人！
学会享受Code Reivew
如何做一个令人尊敬的代码评审人员

当项目维护者不喜欢贡献者提交的代码，经常会出现这种情况。在最好的情况下，这种代码评审策略会导致无意义的争论。在最坏的情况下，它会阻碍项目贡献，形成一个充满敌意和精英主义的文化。

代码评审应该是客观和简洁的，并尽可能面向确定性。代码评审不是关于政治或情感上的争论，而是一个技术问题。代码评审的目标应该是不断进取，提升项目及其参与者的水平。提交的代码应该根据代码的优点而不是对提交者的意见来评判。

代码评审策略

以下是一些代码评审策略，作为项目维护者，如果你看到不喜欢的代码，可以尝试应用这些评审策略。

1. 把拒绝变成问问题

糟糕的评审：“如果你这样改，就不可能……”。（这显然有点夸张，真的不可能吗？）好的评审：“如果你这样改，那该怎么……？”

2. 避免夸大其词

简单一点，把你的顾虑和问题说出来，这样有助于你获得期望的结果。

糟糕的评审：“这个变更对性能影响很大。”好的评审：“这样改的话性能会比之前低一些，你有做过测试吗？”更好的评审：“我会准备一些数据来验证一下这样改之后速度不会比之前慢。”或者这样：“这个变更把之前的 O(n) 变成了 O(n2)，不会影响性能吗？”

3. 把带有讽刺意味的评语留给你自己

有些想法最好把它烂在肚子里，如果你不想让人觉得你粗鲁，最好不要把这些想法说出来。

“我觉得这个变更太糟糕了，它会毁了所有东西的。”“你真的觉得软件工程这个行业适合你呆下去吗？”

4. 积极参与

对于同一个问题，或许你会有不同的想法。如果你能够积极参与，可能会得到比之前更好的解决方案。

糟糕的评审：“这个想法太糟糕了，我有更好的解决方案。”好的评审：“对于这个问题我也有一些类似的想法，或许我们可以比较或者组合一下我们的想法。”或者：“我也有一些类似的想法，我这样做是因为……而你那么做是因为什么？”

5. 不是每个人的经历都跟你一样

有些东西对你来说是常识，但有些人可能并不知道，即使他们的能力并不差。你可以说这些东西是常识，但不要显露出嘲笑或高人一等的姿态。

糟糕的评审：“你不知道这个明显是错的吗？”好的评审：“这样是不对的，因为当……的时候它会抛出空指针异常。”

6. 不要把复杂的东西简单化

有些事情对你来说是显而易见的，但对其他人并不一定也是这样。为别人提供可选方案或有用的例子可以让他们也变得跟你一样好。

糟糕的评审：“为什么不直接这样？”好的评审：“这样做会更简单，比如……”

7. 尊重别人

有时候，提交的代码可能质量很差，但表示一下对别人的尊重其实很容易。

糟糕的评审：“这些愚蠢的代码人跟写代码的人一样愚蠢。”好的评审：“感谢你提交的代码，但我们不能接受它们，因为有一些问题（已经列出来了）。”或者：“代码有一些问题，已经列出来了。或许我们可以回退一步，一起讨论一下用例？这样可以帮我们往前进一步。”

8. 管理你的期望和时间

如果一次提交的代码太多，你没有时间评审，可以让提交者知道。

糟糕的评审：“代码太多了，我不会合并它们的。”同样糟糕的是直接忽略它们。好的评审：“可以把这些分成几次提交吗？我没有这么多时间，而且一次也评审不了这么多代码。”

9. 使用礼貌用语

使用礼貌用语（比如“请”）表示对代码提交者的尊重，特别是当你要他们在细节上做出一些调整（比如格式化）时。

“请你把与空格相关的变更放在单独的 PR 里，可以吗？”“请你把变量对齐，这样更容易阅读，可以吗？”

10. 开启对话

你可能照着上面所说的去做了，但对提交的代码仍然不满意。这个时候你可以这么说：“我不喜欢这段代码，但我也不知道为什么，我们可以聊聊吗？”尽管需要多花一点时间，但这样是值得的，因为这样你和对方都能学到东西（一个讲一个听），而不是变成对立方。

即使是很有经验的程序员也可以这么说：“我也不知道为什么不喜欢这段代码”。这不是在创造攻击代码评审人员的机会，而是打开一条共同求知的道路。

总结

避免使用双关语或夸大其词，避免争论，避免精英主义或贬低他人，避免诸如“显而易见”和“你为什么不……”这样的评语。使用清晰、真实的陈述和支持性的语言，提出问题，推动事情向前发展。记住，同事和代码贡献者都是人，他们的付出和你的付出一样值得尊重。

三、什么时候、如何进行代码审查

3.1、我的建议

1、个人：每一次代码提交时的强制三人左右的代码评审。

2、会审：以项目为单位，召开专门的代码评审会议。

3、团队：代码走查，团队成员互相检查代码，小团队一周一次，大团队一个月一次。

3.2、其他借鉴

https://www.cnblogs.com/DinoFung/articles/2302964.html

四、Code Review 时该关注的

4.1、体系结构和代码设计

代码复用: 根据“三振法”, 如果代码被复制一次，虽然不喜欢这种方式，但通常没什么问题。但如果再一次被复制，就应该通过提取公共的部分来重构它。
用更好的代码: 如果在一块混乱的代码做修改，添加几行代码也许更容易，但建议更进一步，用比原来更好的代码。
潜在的bugs: 是否会引起的其他错误？循环是否以我们期望的方式终止？
错误处理: 错误确定被优雅的修改？会导致其他错误？如果这样，修改是否有用？
效率: 如果代码中包含算法，这种算法是否是高效？例如，在字典中使用迭代，遍历一个期望的值，这是一种低效的方式。
新代码与全局的架构是否保持一致？
基础代码是否有结合使用了一些标准或设计样式，新的代码是否遵循当前的规范？代码是否正确迁移，或参照了因不规范而淘汰的旧代码？
代码的位置是否正确？比如涉及订单的新代码是否在订单服务相关的位置？
新代码是否重用了现存的代码？新代码是否可以被现有代码重用？新代码是否有重复代码？如果是的话，是否应该重构成一个更可被重用的模式，还是当前还可以接受？
新代码是否被过度设计了？是否引入现在还不需要的重用设计？

4.2、可读性和可维护性

字段、变量、参数、方法、类的命名是否真实反映它们所代表的事物, 是否能够望文生义?
是否可以通过读代码理解它做了什么？
是否理解测试用例测了什么？
测试是否很好地覆盖了用例的各种情况？它们是否覆盖了正常和异常用例？是否有忽略的情况？
错误信息是否可被理解？ log打的是否正确和足够?
不清晰的代码是否被文档、注释或容易理解的测试用例所覆盖？具体可以根据团队自身的喜好决定使用哪种方式。

4.3、功能

代码是否真的达到了预期的目标？如果有自动化测试来确保代码的正确性，测试的代码是否真的可以验证代码达到了协定的需求？
代码看上去是否包含不明显的bug，比如使用错误的变量进行检查，或误把and写成or？
作者是否需要创建公共文档或修改现存的帮助文档？
是否检查了面向用户的信息的正确性？
是否有会在生产环境中导致应用停止运行的明显错误？代码是否会错误地指向测试数据库，是否存在应在真实服务中移除的硬编码的stub代码？
你对性能的需求是什么，你是否考虑了安全问题？
1. 这个新增或修复的功能是否会反向影响到现存的性能测试结果
2. 外部调用很昂贵(a. 数据库调用. b. 不必要的远程调用. c. 移动或穿戴设备过频繁地调用后端程序)

4.4、安全

检查是否新的路径和服务需要认证
数据是否需要加密
密码是否被很好地控制？

这里的密码包含密码（如用户密码、数据库密码或其他系统的密码）、秘钥、令牌等等。这些永远不应该存放在会提交到源码控制系统的代码或配置文件中，有其他方式管理这些密码，例如通过密码服务器（secret server）。当审查代码时，要确保这些密码不会悄悄进入你的版本控制系统中
代码的运行是否应该被日志记录或监控？是否正确地使用？
日志和监控需求因各个项目而不同，一些需要合规，一些拥有比别人严格的行为、事件日志规范。如果你有规章规定哪些需要记录日志，何时、如何记录，那么作为代码审查者你应该检查提交的代码是否满足要求。如果你没有固定的规章，那么就考虑：
- 代码是否改变了数据（如增删改操作）？是否应该记录由谁何时改变了什么？
- 代码是否涉及关键性能的部分？是否应该在性能监控系统中记录开始时间和结束时间？
- 每条日志的日志等级是否恰当？一个好的经验法则是“ERROR”触发一个提示发送到某处，如果你不想这些消息在凌晨3点叫醒谁，那么就将之降级为“INFO”或“DEBUG”。当在循环中或一条数据可能产生多条输出的情况下，一般不需要将它们记录到生产日志文件中，它们更应该被放在“DEBUG”级别。

4.5、其他方面

是否合理地释放了资源
1. 是否存在内存泄漏?
2. 是否存在内存无限增长? 例如, 如果审查者看到不断有变量被追加到list或map中, 那么就要考虑下这个list或map什么时候失效, 或清除无用数据
3. 代码是否及时关闭了连接或数据流?
4. 资源池配置是否是否正确? 有没有过大或者过小?
异常情况是否能够正确处理?
1. 超时是否能够正确处理?
2. 调用接口出错的时候, 是否有出错处理逻辑, 并且处理正确?
3. 进程意外重启后, 是否能够恢复到崩溃前的环境?
正确性(主要与多线程环境关系密切)
1. 代码是否使用了正确的适合多线程的数据结构
2. 代码是否存在竞态条件（race conditions）？多线程环境中代码非常容易造成不明显的竞态条件。作为审查者，可以查看不是原子操作的get和set
3. 代码是否正确使用锁？和竞态条件相关，作为审查者你应该检查被审代码是否允许多个线程修改变量导致程序崩溃。代码可能需要同步、锁、原子变量来对代码块进行控制
4. 代码的性能测试是否有价值？很容易将小型的性能测试代码写得很糟糕，或者使用不能代表生产环境数据的测试数据，这样只会得到错误的结果
5. 缓存：虽然缓存是一种能防止过多高消耗请求的方式，但其本身也存在一些挑战。如果审查的代码使用了缓存，你应该关注一些常见的问题，如，不正确的缓存失效方式
代码级优化, 对大部分并不是要构建低延时应用的机构来说，代码级优化往往是过早优化，所以首先要知道代码级优化是否必要
1. 代码是否在不需要的地方使用同步或锁操作？如果代码始终运行在单线程中，锁往往是不必要的
2. 代码是否可以使用原子变量替代锁或同步操作？
3. 代码是否使用了不必要的线程安全的数据结构？比如是否可以使用ArrayList替代Vector？
4. 代码是否在通用的操作中使用了低性能的数据结构？如在经常需要查找某个特定元素的地方使用链表
5. 代码是否可以使用懒加载并从中获得性能提升？
6. 条件判断语句或其他逻辑是否可以将最高效的求值语句放在前面来使其他语句短路？
7. 代码是否存在许多字符串格式化？是否有方法可以使之更高效？
8. 日志语句是否使用了字符串格式化？是否先使用条件判断语句校验了日志等级，或使用延迟求值？