如何使代码审查更高效

要点

• 代码审查者在审查代码时有非常多的东西需要关注。一个团队需要明确对于自己的项目哪些点是重要的，并不断在审查中就这些点进行检查。
• 人工审查代码是十分昂贵的，因此尽可能地使用自动化方式进行审查，如：代码格式、代码样式、检查常见bug、确定常见安全问题以及运行自动化测试。 可以借助sonar工具等进行定期review
• 当针对性能进行审查时，了解系统的性能需求是明确潜在问题的关键。
• 一些简单的人工检查可以显著提升应用的安全性。
• 代码审查是应该在互相沟通中进行讨论的，而不是相互对抗。预先确定哪些是要点哪些不是，可以减少冲突并拟定预期。 比如到达是敲四个空格，还是直接用tab建等相关问题，不应该在代码审查的讨论范围内

关于设计

• 如何让新代码与全局的架构保持一致？也就是上帝的归上帝，凯撒的归凯撒。
• 代码是否遵循SOLID原则，是否遵循团队使用的设计规范，如领域驱动开发等？
• 新代码使用了什么设计模式？这样使用是否合适？
• 基础代码是否有结合使用了一些标准或设计样式，新的代码是否遵循当前的规范？ 参考一些源码规范 代码是否正确迁移，或参照了因不规范而淘汰的旧代码？ 优秀的程序员总是删掉的代码比写的多
• 代码的位置是否正确？比如涉及订单的新代码是否在订单服务相关的位置？
• 新代码是否重用了现存的代码？新代码是否可以被现有代码重用？新代码是否有重复代码？如果是的话，是否应该重构成一个更可被重用的模式，还是当前还可以接受？
• 新代码是否被过度设计了？是否引入现在还不需要的重用设计？团队如何平衡可重用和YAGNI(You Ain’t Gonna Need It)这两种观点？

可读性和可维护性

• 字段、变量、参数、方法、类的命名是否真实反映它们所代表的事物。 可以参考《clean code》一书
• 我是否可以通过读代码理解它做了什么？ 好的代码总是自描述的，而不依赖于注释
• 我是否理解测试用例测了什么？
• 测试是否很好地覆盖了用例的各种情况？它们是否覆盖了正常和异常用例？是否有忽略的情况？
  错误信息是否可被理解？
• 不清晰的代码是否被文档、注释或容易理解的测试用例所覆盖？具体可以根据团队自身的喜好决定使用哪种方式。

关于功能

• 代码看上去是否包含不明显的bug，比如使用错误的变量进行检查，或误把and写成or？ 对于一些常见错误应该有意识，比如非空校验，引用传递保证正确赋值，循环中删除保证不抛出ConcurrentModificationException，校验下标保证不出现数组越界，变量初始化保证正确运行，很多可以借助ide的提示功能保证代码规范，正确运行

关于性能

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

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

• 调用数据库：最坏的情况是问题隐藏在系统抽象中，如关系对象映射（ORM）中。但是在代码审查中你应该可以找到常见的导致性能问题的问题，如在循环中逐个调用数据库，一种情况就是加载ID列表之后，再在数据库中逐个查询ID对应的每条数据。 主要涉及到SQL的优化，减少数据库的查询等
• 不必要的网络调用：就像数据库一样，远程服务有时也会被过度使用，原来只要一个远程调用就可实现的，或者可以使用批量或缓存防止昂贵网络调用的，却使用多个远程调用来实现。再次强调，像数据库一样，有时抽象类会隐藏调用远程API的方法。 减少网络调用

有效且高效地使用资源

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

• 锁是一个性能开销大户，并在多线程环境中很难理清。考虑使用以下模式：单线程写或修改值，其余线程只读，或使用无锁算法。
• 是否存在内存泄露？Java中一些常见的原因会是：可变的静态字段，使用ThreadLocal变量和使用类加载器。
• 是否存在内存无限增长？这个和内存泄露不同，内存泄漏是指无用的对象不能被垃圾回收器回收。但对于任何语言，就算是没有垃圾回收的语言，也能创建无限变大的数据结构。作为审查者，如果你看见新的变量不断被加到list或map中，你就要问下，这个list或map什么时候失效或清除无用数据。
• 代码是否关闭了连接或数据流？关闭连接或文件、网络数据流很容易会被忘记。当你审查别人代码时，如果使用到文件、网络或数据库连接，就要确保它们被正确地关闭了。
• 资源池是否配置正确？针对一个环境的最佳配置取决于很多因素，所以作为审查者你很难马上知道像数据库连接池大小是否正确等这些问题。但是有一些是你一眼就可以看出来的，像资源池是否太小（比如大小设置为1）或太大（如数百万线程）。如果无法确定，就从默认值开始。没有使用默认值的就需要提供一定的测试或计算来证明这么配置的合理性。 包括线程池的设计，使用无界队列还是有界队列，IO密集型可以考虑多些线程来平衡CPU的使用，CPU密集型可以考虑少些线程减少线程调度的消耗。

审查者可以轻松找出的警告信号

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

• 反射：Java的反射比正常调用要慢。如果你在审查含有反射的代码，你就要问下是否必须使用它。
• 超时：当你审查代码时，你可能不知道一个操作合适的超时时间，但是你要想一下“如果超时了，会对系统其他部分造成什么影响？”。作为审查者你应该考虑最坏的情况：当发生5分钟的延时，应用是否会阻塞？如果超时时间设置成1秒钟最坏的情况会是怎么样的？如果代码作者不能确定超时长度，你作为审查者也不知道一个选定的时间的好坏，那么是时候找一个理解这其中影响的人参与代码审查了。 超时时间的设定要合理，做到友好返回
• 并行：代码是否使用多线程来运行一个简单的操作？这样是否花费了更多的时间以及复杂度而并没有提升性能？如果使用现代化的Java，那其中潜在的问题相较于显示创建线程中的问题更不容易被发现：代码是否使用Java 8新的并行流计算但并没有从并行中获益？比如，在少量元素上使用并行流计算，或者只是运行非常简单的操作，这可能比在顺序流上运算还要慢。 涉及到线程间切换，资源竞争问题，这样反而会更加降低性能

正确性

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

• 代码是否使用了正确的适合多线程的数据结构。 比如线程不安全的hashMap应该替换成ConcurrentHashMap
• 代码是否存在竞态条件（race conditions）？多线程环境中代码非常容易造成不明显的竞态条件。作为审查者，可以查看不是原子操作的get和set。
• 代码是否正确使用锁？和竞态条件相关，作为审查者你应该检查被审代码是否允许多个线程修改变量导致程序崩溃。代码可能需要同步、锁、原子变量来对代码块进行控制。
• 代码的性能测试是否有价值？很容易将小型的性能测试代码写得很糟糕，或者使用不能代表生产环境数据的测试数据，这样只会得到错误的结果。
• 缓存：虽然缓存是一种能防止过多高消耗请求的方式，但其本身也存在一些挑战。如果审查的代码使用了缓存，你应该关注一些常见的问题，如，不正确的缓存失效方式。 关注缓存的更新，失效时机

代码级优化

对大部分并不是要构建低延时应用的机构来说，代码级优化往往是过早优化，所以首先要知道代码级优化是否必要。

• 代码是否在不需要的地方使用同步或锁操作？如果代码始终运行在单线程中，锁往往是不必要的。
• 代码是否可以使用原子变量替代锁或同步操作？ volatile关键字
• 代码是否使用了不必要的线程安全的数据结构？比如是否可以使用ArrayList替代Vector？
• 代码是否在通用的操作中使用了低性能的数据结构？如在经常需要查找某个特定元素的地方使用链表。 比如知道集合大小的时候，应该使用带集合大小的构造函数，而不是频繁的扩容
• 代码是否可以使用懒加载并从中获得性能提升？ 相反的，可以用预加载的方式提示性能
• 条件判断语句或其他逻辑是否可以将最高效的求值语句放在前面来使其他语句短路？ 或语句的阶段效应
• 代码是否存在许多字符串格式化？是否有方法可以使之更高效？
• 日志语句是否使用了字符串格式化？是否先使用条件判断语句校验了日志等级，或使用延迟求值？

简单的代码即高效的代码

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

• 短小的方法和类。
• 简单的逻辑，即消除嵌套的条件或循环语句。
  越是人类可读的代码，JIT编译器越有可能理解你的代码并进行优化。这应该在代码审查中很容易定位，如果代码看上去易理解且整洁，它运行时效率也会越好。

关于安全

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

尽可能使用自动化

有惊人数量的安全检查可以被自动化，而不需要人工干预。安全测试不一定要启动所有系统进行完整的渗透测试，一些问题可以在代码级就能被发现。
常见问题如SQL注入或跨站脚本可以在持续集成环境通过相应工具检查出。你也能通过OWASP依赖检测工具自动化检查你依赖中已知的漏洞。

有时需要“看情况”

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

理解你用到的依赖

第三方类库是侵蚀系统安全并引起漏洞的一个途径。当审查代码时至少你要检查是否引入了新的依赖（如第三方类库）。如果你还没有自动化检查漏洞，你应该检查新引入的类库中已知的问题。
你也应该尝试着最小化每个类库的版本，当然如果其他依赖有一个额外的间接依赖，这点可能达不到。但最简单的最小化自己代码暴露在他人代码的（通过类库或服务）安全问题中的方法有：

• 尽可能使用源码并理解它的可信度。
• 使用你所能得到的质量最高的类库。
• 追踪你在何处使用了什么，当新的漏洞出现，你可以查看你受影响的程度。

检查是否新的路径和服务需要认证

无论你开发web应用、提供web服务或一些其他需要认证的API，当你增加一个新的URI或服务时，你应该确保它不能在没有认证的情况下被访问（假设认证是你系统的需求）。你只要简单地检查代码的开发者写了合适的测试用例来展示进行了认证。 访问的合法性校验，特别是内部系统，不应该暴露公网地址，即使有公网地址暴露，也应该有身份、权限校验

你应该不只针对使用用户名和密码的人类用户来考虑认证。其他系统或自动化流程来访问你的应用或服务也会需要认证。这可能影响你们系统中对“用户”的定义。 也就是服务治理过程中，保证每个调用你的服务是有注册过的

数据是否需要加密

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

密码是否被很好地控制？

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

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

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

• 代码是否改变了数据（如增删改操作）？是否应该记录由谁何时改变了什么？ 重要信息的变更不仅仅只是有身份校验，也应该有日志记录，保证将来有迹可循
• 代码是否涉及关键性能的部分？是否应该在性能监控系统中记录开始时间和结束时间？ 重要路径性能打点
• 每条日志的日志等级是否恰当？一个好的经验法则是“ERROR”触发一个提示发送到某处，如果你不想这些消息在凌晨3点叫醒谁，那么就将之降级为“INFO”或“DEBUG”。当在循环中或一条数据可能产生多条输出的情况下，一般不需要将它们记录到生产日志文件中，它们更应该被放在“DEBUG”级别。

写在最后

代码审查是一个很好的方式，不仅确保了代码质量和一致性，也在团队中或团队间分享了项目知识。即使你已经自动化了基础的校验，还有许多不同代码、设计的方面需要考虑。代码审查工具，如Upsource，通过在每个代码提交的检查中高亮可疑的代码并分析哪些问题已经被修复，新引入哪些问题，可以帮你定位一些潜在的问题。工具也可以简化流程，因为它提供了一个平台来讨论设计和代码实现，也可以邀请审查者、作者和其他相关人员参加讨论直到达成共识。
最后，团队需要花时间决定代码质量的哪些因素对他们是重要的，也需要专家人工决定哪些规则应用到各个代码审查中，参与到审查中的每个人都应该具备并使用人际交往的技巧，如积极的反馈、谈判妥协以达到最终的共识，即代码应该怎么样才“足够好”可以通过审查。