代码审查如何做

做Code Review的正反方观点

首先，我们先来看看正方的观点：

代码审查中，可以通过大家的建议增进代码的质量。
代码审查是一个传递知识的手段，可以让其它并不熟悉代码的人知道作者的意图和想法，从而可以在以后轻松维护代码。
代码审查鼓励程序员们相互学习对方的长处和优点。
代码审查可以被用来确认自己的设计和实现是清楚和简单的。

我们再来听下反方的声音：

很多开发团队抱怨代码审查就是一个形式，费时费力不说，发现的问题还不如测试，而评审者们除了在代码风格上有些见术，别的也就没什么用了，长而久之，大家都会开始厌烦这个事了。

那么为了这篇文章能够写下去，笔者显然是属于正方了，此处不再做争辩。

回顾时，什么关注，什么不该关注

在上面的Code Reivew中的正方观点中，我们没有提到可以帮助找到程序的bug和保证代码风格和编码标准，那么是说Code Review不能帮助改善这几方面吗？不是的，这里确切地说，Code Review不是万能的，在执行的时候，应该专注于它应该关注的部分，那么问题来了，Code Review时，哪些是该关注的，哪些又是不该关注的呢？

Code Review时不该关注的

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

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

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

Code Review时该关注的

代码审查者在审查代码时有非常多的东西需要关注。一个团队需要明确对于自己的项目哪些点是重要的，并不断在审查中就这些点进行检查。

1.体系结构和代码设计

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

2.可读性和可维护性

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

3.功能

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

安全

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

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

5.其他方面

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

Code Review的实际操作建议

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

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