如何作Code Review

1. 引言

众所周知，在团队中进行代码审查（Code Review）能够提高代码质量，分享项目知识、明确责任，最终达到构建更好的软件、更好的团队。若是你花几秒钟搜索代码审查的相关信息，你会看到许多关于代码审查带来的价值的文章。也有许多方法来进行代码审查：在GitHub中提pull request，或使用像JetBrains的Upsource之类的工具。然而即便拥有清晰的流程和正确的工具，还遗留了一个大问题须要解决——咱们须要找寻哪些问题。

可能没有明确关于“咱们须要找寻哪些问题”的文章，是由于有许多不一样的要点须要考虑。正如任何其余的需求，各个团队对各个方面都有不一样的优先级。

本文的目标是列出一些审查者能够找寻的要点，而各个方面的优先级就因各个团队而异了。

在咱们继续以前，让咱们考虑一下你们在代码审查时会讨论到的问题。对于代码的格式、样式和命名以及缺乏测试这些问题是很常见的几点。若是你想拥有可持续的、可维护的代码，这些是有用的检查点。然而，在代码审查时讨论这些就有些浪费时间，由于不少这样的检查能够（也应该）被自动化。

那哪些要点是只能由人工进行审查而不能依靠工具的呢？

回答是有惊人数量的点只能由人工进行审查。在本文剩下的部分，咱们会覆盖一系列普遍的特性，并深刻其中的两点具体的区域：性能和安全。

2. 设计规范

• 如何让新代码与全局的架构保持一致？

• 代码是否遵循SOLID原则，是否遵循团队使用的设计规范，如领域驱动开发等？

• 新代码使用了什么设计模式？这样使用是否合适？

• 基础代码是否有结合使用了一些标准或设计样式，新的代码是否遵循当前的规范？代码是否正确迁移，或参照了因不规范而淘汰的旧代码？

• 代码的位置是否正确？好比涉及订单的新代码是否在订单服务相关的位置？

• 新代码是否重用了现存的代码？新代码是否能够被现有代码重用？新代码是否有重复代码？若是是的话，是否应该重构成一个更可被重用的模式，仍是当前还能够接受？

• 新代码是否被过分设计了？是否引入如今还不须要的重用设计？团队如何平衡可重用和YAGNI(You Ain’t Gonna Need It)这两种观点？

3. 可读性和可维护性

• 字段、变量、参数、方法、类的命名是否真实反映它们所表明的事物。

• 我是否能够经过读代码理解它作了什么？

• 我是否理解测试用例测了什么？

• 测试是否很好地覆盖了用例的各类状况？它们是否覆盖了正常和异经常使用例？是否有忽略的状况？

• 错误信息是否可被理解？

• 不清晰的代码是否被文档、注释或容易理解的测试用例所覆盖？具体能够根据团队自身的喜爱决定使用哪一种方式。

4. 功能

• 代码是否真的达到了预期的目标？若是有自动化测试来确保代码的正确性，测试的代码是否真的能够验证代码达到了协定的需求？

• 代码看上去是否包含不明显的bug，好比使用错误的变量进行检查，或误把and写成or？

5. 深思熟虑

• 是否须要知足相关监管需求？

• 做者是否须要建立公共文档或修改现存的帮助文档？

• 是否检查了面向用户的信息的正确性？

• 是否有会在生产环境中致使应用中止运行的明显错误？代码是否会错误地指向测试数据库，是否存在应在真实服务中移除的硬编码的stub代码？

• 你对性能的需求是什么，你是否考虑了安全问题？这些是须要覆盖到的重大区域也是相当重要的话题。

    让咱们深刻探讨下性能，这是一个真正能从代码审查中获益的方面。

    系统对性能方面的非功能性需求应当同全部架构、设计的领域同样被置于重要位置。不管你是开发只允许纳秒级延时的低延迟交易系统，仍是管理“待办事项”的手机应用，你都应该了解用户所认为的“太慢”。

    在考虑咱们是否须要就代码性能进行代码审查以前，咱们应该问本身几个关于具体需求是什么的问题。虽然一些应用确实不须要考虑每毫秒都花费在哪里，对于大部分应用，花费几个小时的折腾进行优化来得到的些许CPU降低的价值也是有限的，但有些地方仍是审查者能够检查一下的，进而确保代码不会有一些常见可避免的性能缺陷。

5.1 这段代码是否有硬性的性能需求？

接受审查的代码是否涉及以前发布的服务等级协议（SLA）？或这个需求自己有特别的性能需求？

若是代码致使“登陆页面加载太慢”，那原始的开发者须要找出合适的加载时间是多久，否则审查者或做者本人如何确保改进后的速度足够快？

若是有硬性的需求，是否有测试能证实知足了该需求？任何注重性能的系统应该就性能提供自动化测试，这能确保发布的SLA达到预期（如全部订单请求要在10毫秒内处理）。没有这些，你只能依靠你的用户来告诉你没有达到对应的SLA。这不只是一种糟糕的用户体验，还会带来本来可避免的罚金和支出。

5.2 这个修复或新增的功能是否会反向影响到任何现存的性能测试结果

若是你按期运行性能测试或有测试套件能够按需运行它们，那你就须要检查新的代码是否使得性能关键区域的系统性能有所降低。这能够是一个自动化的流程，但因为在持续集成环境中更常运行单元测试而不是性能测试，因此值得特别指出能够在代码审查中检查这项。

5.3 调用外部的服务或应用的代价是昂贵的

任何经过网路来使用外部系统的方式一般会比没有很好优化的方法有更差的性能。考虑如下几点：

1. 调用数据库：最坏的状况是问题隐藏在系统抽象中，如关系对象映射（ORM）中。可是在代码审查中你应该能够找到常见的致使性能问题的问题，如在循环中逐个调用数据库，一种状况就是加载ID列表以后，再在数据库中逐个查询ID对应的每条数据。 

2. 没必要要的网络调用：就像数据库同样，远程服务有时也会被过分使用，原来只要一个远程调用就可实现的，或者可使用批量或缓存防止昂贵网络调用的，却使用多个远程调用来实现。再次强调，像数据库同样，有时抽象类会隐藏调用远程API的方法。 

3. 移动或可穿戴应用过于频繁地调用后端程序：这基本上和“没必要要的网络调用”相同，可是在移动设备上会产生其余问题，这不只会产生没必要要的调用后端使得性能变差，还会更快地消耗电量甚至致使用户的金钱支出。

5.4 有效且高效地使用资源

    代码是否用锁来控制共享资源的访问？这是否会致使性能下降或死锁？

    锁是一个性能开销大户，并在多线程环境中很难理清。考虑使用如下模式：单线程写或修改值，其他线程只读，或使用无锁算法。

1. 是否存在内存泄露？Java中一些常见的缘由会是：可变的静态字段，使用ThreadLocal变量和使用类加载器。 

2. 是否存在内存无限增加？这个和内存泄露不一样，内存泄漏是指无用的对象不能被垃圾回收器回收。但对于任何语言，就算是没有垃圾回收的语言，也能建立无限变大的数据结构。做为审查者，若是你看见新的变量不断被加到list或map中，你就要问下，这个list或map何时失效或清除无用数据。

3. 代码是否关闭了链接或数据流？关闭链接或文件、网络数据流很容易会被忘记。当你审查别人代码时，若是使用到文件、网络或数据库链接，就要确保它们被正确地关闭了。

4. 资源池是否配置正确？针对一个环境的最佳配置取决于不少因素，因此做为审查者你很难立刻知道像数据库链接池大小是否正确等这些问题。可是有一些是你一眼就能够看出来的，像资源池是否过小（好比大小设置为1）或太大（如数百万线程）。若是没法肯定，就从默认值开始。没有使用默认值的就须要提供必定的测试或计算来证实这么配置的合理性。

5.5 审查者能够轻松找出的警告信号

    一些代码一眼就能看出存在潜在性能问题。这依赖于所使用的语言和类库。

• 反射：Java的反射比正常调用要慢。若是你在审查含有反射的代码，你就要问下是否必须使用它。

• 超时：当你审查代码时，你可能不知道一个操做合适的超时时间，可是你要想一下“若是超时了，会对系统其余部分形成什么影响？”。做为审查者你应该考虑最坏的状况：当发生5分钟的延时，应用是否会阻塞？若是超时时间设置成1秒钟最坏的状况会是怎么样的？若是代码做者不能肯定超时长度，你做为审查者也不知道一个选定的时间的好坏，那么是时候找一个理解这其中影响的人参与代码审查了。

• 并行：代码是否使用多线程来运行一个简单的操做？这样是否花费了更多的时间以及复杂度而并无提高性能？若是使用现代化的Java，那其中潜在的问题相较于显示建立线程中的问题更不容易被发现：代码是否使用Java 8新的并行流计算但并无从并行中获益？好比，在少许元素上使用并行流计算，或者只是运行很是简单的操做，这可能比在顺序流上运算还要慢。

5.6 正确性

      这些不必定影响系统的性能，可是它们与多线程环境运行关系密切，因此和这个主题有关：

• 代码是否使用了正确的适合多线程的数据结构。

• 代码是否存在竞态条件（race conditions）？多线程环境中代码很是容易形成不明显的竞态条件。做为审查者，能够查看不是原子操做的get和set。

• 代码是否正确使用锁？和竞态条件相关，做为审查者你应该检查被审代码是否容许多个线程修改变量致使程序崩溃。代码可能须要同步、锁、原子变量来对代码块进行控制。

• 代码的性能测试是否有价值？很容易将小型的性能测试代码写得很糟糕，或者使用不能表明生产环境数据的测试数据，这样只会获得错误的结果。

• 缓存：虽然缓存是一种能防止过多高消耗请求的方式，但其自己也存在一些挑战。若是审查的代码使用了缓存，你应该关注一些常见的问题，如，不正确的缓存失效方式。

5.7 代码级优化

对大部分并非要构建低延时应用的机构来讲，代码级优化每每是过早优化，因此首先要知道代码级优化是否必要。

• 代码是否在不须要的地方使用同步或锁操做？若是代码始终运行在单线程中，锁每每是没必要要的。

• 代码是否可使用原子变量替代锁或同步操做？

• 代码是否使用了没必要要的线程安全的数据结构？好比是否可使用ArrayList替代Vector？

• 代码是否在通用的操做中使用了低性能的数据结构？如在常常须要查找某个特定元素的地方使用链表。

• 代码是否可使用懒加载并从中得到性能提高？

• 条件判断语句或其余逻辑是否能够将最高效的求值语句放在前面来使其余语句短路？

• 代码是否存在许多字符串格式化？是否有方法可使之更高效？

• 日志语句是否使用了字符串格式化？是否先使用条件判断语句校验了日志等级，或使用延迟求值？

5.8 简单的代码即高效的代码

Java代码中有一些简单的东西能够供审查者寻找，这些会使JVM很好地替你优化你的代码：

• 短小的方法和类。

• 简单的逻辑，即消除嵌套的条件或循环语句。

5.9 安全

你在构建一个安全、稳固的系统所花费的精力，和花在其余特性上的同样，取决于项目自己，项目运行的地方、它使用的用户、须要访问的数据等。咱们如今着重看一些你可能在代码审查时关注的地方。

5.10 尽量使用自动化

有惊人数量的安全检查能够被自动化，而不须要人工干预。安全测试不必定要启动全部系统进行完整的渗透测试，一些问题能够在代码级就能被发现。

常见问题如SQL注入或跨站脚本能够在持续集成环境经过相应工具检查出。你也能经过OWASP依赖检测工具自动化检查你依赖中已知的漏洞。

5.11 有时须要“看状况”

对有的校验你能够简单回答“是”或“否”，有时你须要一个工具指出潜在的问题，以后再由人工来决定这个是否须要解决。这也正是Upsource真正的闪光点。Upsource显示代码检查结果，审查者能够利用这些来决定代码是否须要改动或还能够接受目前的状况。

5.12 理解你用到的依赖

第三方类库是侵蚀系统安全并引发漏洞的一个途径。当审查代码时至少你要检查是否引入了新的依赖（如第三方类库）。若是你尚未自动化检查漏洞，你应该检查新引入的类库中已知的问题。

你也应该尝试着最小化每一个类库的版本，固然若是其余依赖有一个额外的间接依赖，这点可能达不到。但最简单的最小化本身代码暴露在他人代码的（经过类库或服务）安全问题中的方法有：

• 尽量使用源码并理解它的可信度。

• 使用你所能获得的质量最高的类库。 

• 追踪你在何处使用了什么，当新的漏洞出现，你能够查看你受影响的程度。

5.13 ​​​​​​​检查是否新的路径和服务须要认证

不管你开发web应用、提供web服务或一些其余须要认证的API，当你增长一个新的URI或服务时，你应该确保它不能在没有认证的状况下被访问（假设认证是你系统的需求）。你只要简单地检查代码的开发者写了合适的测试用例来展现进行了认证。

你应该不仅针对使用用户名和密码的人类用户来考虑认证。其余系统或自动化流程来访问你的应用或服务也会须要认证。这可能影响大家系统中对“用户”的定义。

5.14 ​​​​​​​数据是否须要加密

当你保存一些数据到磁盘或经过线缆传输，你须要了解数据是否应该被加密。显然密码永远不该该是简单文本，可是有诸多其余状况数据须要加密。若是被审查的代码经过线缆来传送数据或保存在某地或以其余方式离开你的系统，且你不知道它是否应该被加密，尝试询问下你组织中能够回答这个问题的人。

5.15 ​​​密码是否被很好地控制？

这里的密码包含密码（如用户密码、数据库密码或其余系统的密码）、秘钥、令牌等等。这些永远不该该存放在会提交到源码控制系统的代码或配置文件中，有其余方式管理这些密码，例如经过密码服务器（secret server）。当审查代码时，要确保这些密码不会悄悄进入你的版本控制系统中。

5.16 ​​​​​​​代码的运行是否应该被日志记录或监控？是否正确地使用？

日志和监控需求因各个项目而不一样，一些须要合规，一些拥有比别人严格的行为、事件日志规范。若是你有规章规定哪些须要记录日志，什么时候、如何记录，那么做为代码审查者你应该检查提交的代码是否知足要求。若是你没有固定的规章，那么就考虑：

• 代码是否改变了数据（如增删改操做）？是否应该记录由谁什么时候改变了什么？

• 代码是否涉及关键性能的部分？是否应该在性能监控系统中记录开始时间和结束时间？

• 每条日志的日志等级是否恰当？一个好的经验法则是“ERROR”触发一个提示发送到某处，若是你不想这些消息在凌晨3点叫醒谁，那么就将之降级为“INFO”或“DEBUG”。当在循环中或一条数据可能产生多条输出的状况下，通常不须要将它们记录到生产日志文件中，它们更应该被放在“DEBUG”级别。

5.17 ​​​​​​​记得叫上专家

安全是个很大的话题，大到足以让你的公司聘请技术安全专家。咱们有安全专家就能够得到他们的帮助，如，邀请他们参加代码审查，或邀请他们在审查代码时和咱们结对。若是这个没法实现，咱们能够充分学习咱们系统的环境，来理解咱们有哪一种安全需求（面向内部的企业级应用和面向客户的网页应用有不一样的标准），因此咱们能够更好地理解咱们应该在代码审查中看什么。

6. 总结

    代码审查是一个很好的方式，不只确保了代码质量和一致性，也在团队中或团队间分享了项目知识。即便你已经自动化了基础的校验，还有许多不一样代码、设计的方面须要考虑。代码审查工具，如Upsource，经过在每一个代码提交的检查中高亮可疑的代码并分析哪些问题已经被修复，新引入哪些问题，能够帮你定位一些潜在的问题。工具也能够简化流程，由于它提供了一个平台来讨论设计和代码实现，也能够邀请审查者、做者和其余相关人员参加讨论直到达成共识。

    最后，团队须要花时间决定代码质量的哪些因素对他们是重要的，也须要专家人工决定哪些规则应用到各个代码审查中，参与到审查中的每一个人都应该具有并使用人际交往的技巧，如积极的反馈、谈判妥协以达到最终的共识，即代码应该怎么样才“足够好”能够经过审查。