让我们来谈谈代码审查。如果你花几分钟时间搜索有关代码审查的信息，你会看到很多文章讨论为什么代码审查是一件好事.

你还会看到很多关于如何使用代码审查工具的文档，比如 Upsource。但你不太容易找到一份指导你在审查他人代码时应该关注什么的指南。

可能没有明确文章解释应该寻找什么的原因是：有很多不同的事情需要考虑。并且，像任何一组要求（功能性的或非功能性的）一样，不同的组织对于每个方面都会有不同的优先级。

由于这是一个很大的主题，本章的目标只是概述在执行代码审查时，审阅者可能会关注的一些事情。确定每个方面的优先级并保持一致性检查是一个足够复杂的话题，足以成为一个独立的章节。

在审查他人的代码时，你寻找什么？

无论你是通过 Upsource 这样的工具还是在与同事一起走查他们的代码，无论情况如何，有些东西比其他东西更容易评论。一些例子：

• 格式化：空格和换行在哪里？他们使用制表符还是空格？花括号是如何排列的？
• 风格：变量/参数是否被声明为 final？方法变量是在它们使用的代码附近定义还是在方法的开始处定义？
• 命名：字段/常量/变量/类名的命名是否符合标准？名称过长吗？
• 测试覆盖率：有没有针对这段代码的测试？这些都是有效的检查事项——你希望最小化在不同代码区域之间切换上下文，减少认知负荷，所以你的代码看起来越一致越好。

然而，让人类去找这些可能不是在你的组织中最佳的时间和资源的使用方式，因为许多这些检查都可以自动化。有很多工具可以确保您的代码格式保持一致，遵循命名和 final 关键字使用的标准，并且找出由简单的编程错误引起的常见错误。例如，您可以从命令行运行 IntelliJ IDEA 的检查，这样你就不必依赖所有团队成员在 IDE 中运行相同的检查。

http://blog.codinghorror.com/code-reviews-just-do-it/

https://en.wikipedia.org/wiki/Cognitive_load

你应该寻找什么

人类真正擅长的是什么？在代码审查中，我们能注意到哪些事情是工具无法委托的？

结果是，有很多事情。这绝对不是一份详尽无遗的清单，我们也不会在这里详细讨论其中的任何一个。相反，这应该是你组织内关于目前你在代码审查中寻找哪些事情的对话的开始，以及可能你还应该寻找什么。

设计

• 新代码如何与整体架构相匹配？
• 代码是否遵循 SOLID 原则、领域驱动设计 ® 和其他团队青睐的设计范式？
• 在新代码中使用了哪些设计模式？这些是否适当？
• 如果代码库混合了标准或设计风格，新代码是否遵循当前的实践？代码是否朝着正确的方向迁移，还是它模仿了应该逐步淘汰的旧代码？
• 代码是否位于正确的地方？例如，如果代码与订单相关，它是位于订单服务中吗？
• 新代码是否有在现有代码中复用某种东西的可能性？新代码是否提供了可以在现有代码中复用的功能？新代码是否引入了重复内容？如果是的话，应该将其重构为更可重用的模式，还是在这个阶段可以接受？
• 代码是否过度设计？它为了不必要的可复用性而构建了吗？团队是如何平衡考虑可复用性与 YAGNI10 的？

https://www.jetbrains.com/idea/help/running-inspections-offline.html
https://en.wikipedia.org/wiki/SOLID_(object-oriented_design)
https://en.wikipedia.org/wiki/Domain-driven_design
https://en.wikipedia.org/wiki/Software_design_pattern
https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

可读性和可维护性

• 字段、变量、参数、方法和类名的命名是否真正反映了它们代表的实体？
• 通过阅读代码，我能否理解代码的功能？
• 我能理解测试的功能吗？
• 测试覆盖了好的案例子集吗？它们涵盖了正常路径和异常情况吗？是否有未被考虑的案例？
• 异常错误消息是否容易理解？
• 是否有令人困惑的代码部分已经被文档化、注释或者被可理解的测试（根据团队的偏好）所涵盖？

功能性

• 代码实际上是否做到了它应该做的事情？如果有自动化测试来确保代码的正确性，这些测试是否真的测试了代码是否符合约定的要求？
• 代码看起来是否含有潜在的细微错误，比如使用错误的变量进行检查，或者是无意中使用“和”而不是“或”？

你有考虑过…

• 代码中是否有潜在的安全问题？
• 是否需要满足监管要求？
• 对于没有自动化性能测试覆盖的区域，新代码是否引入了可避免的性能问题，比如不必要的数据库调用或远程服务调用？
• 作者是否需要创建公共文档，或者更改现有的帮助文件？
• 用户界面消息是否已经被检查过正确性？
• 是否有明显的错误会在生产环境中停止工作？代码是否会不小心指向测试数据库，或者是是否有硬编码的占位符应该被替换为实际的服务？

测试

在前一章中，我们讨论了在代码审查中可以寻找的广泛内容。现在我们将重点放在一个领域：应该关注测试代码的哪些方面。

我们假设：“你的团队已经为代码编写了自动化测试。 测试会在持续集成（CI）环境中定期运行。正在被审查的代码已经通过了自动编译/测试过程。”

提问

在这一章中，我们将探讨在代码审查中查看测试时审阅者可能会考虑的一些事项。

是否有对新/修改后的代码进行测试？

对于新的或修改后的代码来说，无论是修复错误还是添加新功能，都很少不需要一个新或更新的测试来覆盖它。即使是出于“非功能性”原因的更改，如性能提升，也经常可以通过测试来证明。如果代码审查中没有包含任何测试，作为审阅者，你首先应该问的问题是：“为什么不？”

测试是否至少覆盖了复杂的代码部分？

超越简单地“是否有测试”的问题，我们需要回答的是：“重要的代码是否至少被一个测试所覆盖？”

检查测试覆盖率当然是我们可以自动化的事情。但我们不仅仅可以检查特定的百分比覆盖率：我们还可以使用覆盖率工具来确保正确的代码区域被覆盖。

考虑以下例子：

xxxxxxxxxxxxx

我能否理解测试？

拥有提供充分覆盖率的测试是一件事情，但如果连我也作为人类都无法理解这些测试，它们的使用价值就有限了。当测试失败时会发生什么？将很难知道如何修复它们。

考虑以下情况：

测试是否与要求相符？

这是一个真正需要人类专业知识的地方。无论是被审查的代码满足的要求编码在某个正式文件中，还是位于用户故事的卡片上，或者是用户提出的错误报告内，正在被审查的代码应该与某些初始要求相关。

审阅者应该找到原始要求并查看：

• 测试是否与其相符，无论它们是单元测试、端到端测试还是其他类型。例如，如果要求是“应该在密码字段中允许特殊字符‘#’、‘!’和‘&’”，那么应该有一个使用这些值的密码字段的测试。如果测试使用了不同的特殊字符，那么它不能证明代码符合标准。
• 测试覆盖了所有提到的标准。在我们的特殊字符例子中，要求可能继续说“…并且如果使用了其他特殊字符，则向用户提供错误消息”。在这里，审阅者应该检查是否有针对使用无效字符时会发生什么的测试。

我能否想到现有测试未覆盖的情况？

通常我们的要求并没有明确规定。在这些情况下，审阅者应该考虑原始错误/问题/故事中没有被覆盖的边缘情况。

例如，如果我们新的功能是“赋予用户登录系统的能力”，审阅者可能会想，“如果用户输入 null 作为用户名会怎样？”或者“如果用户不存在于系统中会发生什么样的错误？”。如果在被审查的代码中存在这些测试，那么审阅者就可以增加对代码本身处理这些情况的信心。如果这些异常情况的测试不存在，那么审阅者必须检查代码以确定它们是否已得到处理。

如果代码存在但测试没有，这取决于你的团队来决定政策——你是否要求作者添加这些测试？或者你满意于代码审查证明了边缘情况被覆盖？

是否有测试来记录代码的限制？

作为一个审阅者，往往可以看到正在审查的代码中的限制。这些限制有时是故意的——例如，一个只能处理每个批次最多 1000 项的批量处理过程。

一种记录这些故意限制的方法就是明确地测试它们。在上面的例子中，我们可能有一个测试来证明如果你的批次大小超过 1000 会抛出某种异常。

不是强制的必须在自动化测试中表达这些限制，但如果作者写了一个显示他们已实施的限制的测试，那么有这个测试就意味着这些限制是故意的（并且被记录了），而不是仅仅是一个疏忽。

代码审查中的测试是否是正确的类型/级别？

例如，作者是在做昂贵的集成测试，而单元测试可能就足够了吗？他们有没有编写那些在持续集成（CI）环境中无法有效或一致运行的性能微基准测试？

理想情况下，你的自动化测试应该尽可能地快地运行，这意味着不需要进行昂贵的端到端测试来检查所有类型的特性。一个执行某些数学功能或布尔逻辑检查的方法似乎是方法级单元测试的良好候选。

是否有针对安全方面的测试？

安全性是代码审查可以真正带来好处的领域之一。我们会在以后专门写一篇关于安全性的文章，但在测试话题上，我们可以为许多常见问题编写测试。例如，如果我们正在编写上述的登录代码，我们可能还想要写一个测试来显示如果不能首先进行身份验证，我们就不能进入网站的受保护区域（或调用受保护的 API 方法）。

性能测试

在前一章中，我提到了性能作为审阅者可能会检查的一个领域。自动化性能测试显然是本章可以探索的另一类测试，但我将把这些类型测试的讨论留到以后关于代码审查中的特定性能方面的章节。

审阅者也可以编写测试

不同的组织对代码审查有不同的方法。有时非常清楚作者负责做出所有必要的代码更改；有时则更具有合作性，审阅者自己提交对代码的建议。

无论你采取哪种方法，作为一个审阅者，你可能发现编写一些额外的测试来检查审查中的代码非常有价值，这对于理解那片代码来说就像启动 UI 并与一个新特性互动一样有价值。某些方法和代码审查工具使实验代码变得更加容易，而其他则不那么容易。团队的利益在于尽可能轻松地查看和玩转代码，进行代码审查。

提交这些额外测试作为审查的一部分可能是有价值的，但同样也可能是没有必要的，例如如果实验已经给了我，作为审阅者，满意的答案来回答我的问题的话。

总结

无论你的组织如何处理代码审查过程，进行代码审查都有许多优势。你可以在代码集成到主代码库之前，在它仍然容易修复并且上下文仍在开发者的大脑中时，使用代码审查来发现潜在的代码问题。

作为一个代码审阅者，你应该检查原始开发人员是否思考过他们的代码可能的使用方式、可能在哪些条件下会出错，并处理了边缘情况，可能通过自动化测试“文档化”预期的行为（在正常使用和异常情况下）。

如果审阅者寻找测试的存在性并检查测试的正确性，作为一个团队，你就可以对代码的功能有相当高的信心。此外，如果这些测试在一个 CI 环境中定期运行，你可以看到代码持续工作——它们提供了自动回归检测。如果代码审阅者高度重视他们正在审查的代码的良好质量测试，那么这项代码审查的价值在审阅者点击“接受”按钮之后也会继续存在。

性能

在这一章中，我们将讨论在代码审查过程中可以寻找哪些关于被审查代码性能的问题。

就像所有架构/设计领域一样，系统性能的非功能性要求应该事先设定。无论你是在开发一个必须以纳秒响应的低延迟交易系统，还是在创建一个需要响应用户的在线购物网站，或者你在编写一个管理“待办事项”列表的手机应用程序，你应该有一些关于什么被认为是“太慢”的概念。

让我们来看看影响性能的一些因素，审阅者在代码审查过程中可以寻找这些因素。

性能要求

在我们决定是否基于性能进行代码审查之前，我们应该问自己几个问题。

这部分功能是否有硬性的性能要求？

被审查的这段代码是否属于之前已经公布 SLA（服务等级协议）的区域？或者要求中是否说明了所需性能特性？

如果原始要求是一个“登录屏幕加载太慢”的错误报告，那么原始开发者应该澄清一个合适的加载时间是多少——否则审阅者或作者如何能够确信速度得到了足够的改进？

如果有硬性的性能要求，那么是否存在一个测试来证明它满足那些要求？

任何性能关键的系统都应该有自动化性能测试来确保发布 SLAs（例如：在 10 毫秒内处理完所有请求）得以实现。如果没有这些测试，你只能依靠用户告诉你没有达到你的 SLA 的情况。这不仅是一个糟糕的用户体验，还可能导致可以避免的罚款和费用。本系列最后一篇帖子详细介绍了代码审查中的测试。

修复/新功能是否影响现有性能测试的结果？

如果你经常运行性能测试（或者你有可以在需要时运行的套件），检查新的代码在性能关键区域是否引入了系统性能下降的情况。这可能是一个自动化过程，但是由于作为 CI 环境一部分运行性能测试远不如运行单元测试常见，所以在审查中特别提到这一步骤是有价值的。

如果没有硬性的性能要求呢？

花几个小时痛苦地思考那些能为你节省几个 CPU 周期的优化是没有多大价值的。但审阅者可以检查一些事情以确保代码不会受到完全可避免的常见性能陷阱的影响。查看其余列表以了解是否有一些易于取胜的方法来防止未来的性能问题。

服务的/应用程序之外调用是昂贵的

任何需要网络跳转来使用你应用之外的系统都是一般会比你一个优化不佳的 equals() 方法要耗费更多成本。考虑：

数据库调用

最严重的违规者可能隐藏在像 ORMs（对象关系映射器）这样的抽象背后。但在代码审查中，你应该能够捕捉到性能问题的常见原因，比如在一个循环中调用的数据库单个调用——例如，加载一个 ID 列表，然后对每个对应的 ID 查询数据库。

数据库调用

例如，上面的第 19 行可能看起来相当无辜，但它位于一个 for 循环内——你不知道这段代码可能导致多少次数据库调用。

不必要的网络调用

像数据库一样，远程服务有时可能会过度使用，有多个远程调用可能只需要一个，或者批处理或缓存可以防止昂贵的网络调用。同样地，像数据库一样，有时候抽象会隐藏实际上方法调用是在调用远程 API。

移动/可穿戴应用程序过多调用后端

这基本上与“不必要的网络调用”相同，但还增加了在移动设备上，不必要地调用后端不仅会降低性能，还会消耗电池的问题。

高效使用资源

在讨论我们如何使用我们的网络资源之后，审阅者可以查看其他资源的用法来识别可能存在的性能问题。

代码是否使用锁来访问共享资源？这可能导致性能下降或死锁？

锁是性能杀手 11，并且在多线程环境中很难推理。可以考虑以下模式；只有一个线程写入/改变值，而所有其他线程都可以自由读取；或者使用无锁算法 12。

代码中是否有可能引起内存泄露的东西？

在 Java 中，一些常见的原因可以是：可变的静态字段、使用 ThreadLocal 以及使用 ClassLoader13。

应用程序的内存占用有无限增长的可能性吗？

这不同于内存泄露——内存泄露是未使用的对象不能被垃圾回收器收集。但任何语言，甚至是非垃圾回收的语言，都可以创建

11 http://mechanical-sympathy.blogspot.com.es/2013/08/lock-based-vs-lock-free-concurrent.html

12 https://en.wikipedia.org/wiki/Non-blocking_algorithm

13 http://docs.oracle.com/javase/8/docs/api/java/lang/ClassLoader.html

数据结构无限增长。如果作为审阅者，你看到列表或映射中不断地添加新值，询问列表或映射何时被丢弃或修剪。

无限内存占用

在上面的代码审查中，我们可以看到所有来自 Twitter 的消息都被添加到一个映射中。如果我们更全面地检查这个类，我们会发现 allTwitterUsers 映射永远不会被修剪，TwitterUser 中的推文列表也没有被清理。取决于我们正在监控多少用户以及我们多久添加一次推文，这个映射可能会变得非常大，速度非常快。

代码是否关闭了连接/流？

忘记关闭连接或文件/网络流是很常见的。当你审查他人的代码时，无论使用什么语言，如果你正在使用的文件、网络或数据库连接没有被正确关闭，请确保它们被正确关闭。

关闭资源

原始代码作者很容易错过这个问题，因为上面的代码可以顺利编译。作为审阅者，你应该注意在方法退出之前连接、语句和结果集需要关闭。在 Java 7 中，由于 try-with-resources14 的存在，这变得容易管理了许多。下面的截图显示了一个代码审查的结果，其中作者已经将代码更改为使用 try-with-resources。

14 https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

try-with-resources

资源池是否配置正确？

一个环境的最佳配置将取决于许多因素，因此作为审阅者，你可能不会立即知道例如数据库连接池的大小是否合适。但你可以一眼看出一些事情，例如是否太小（比如设置为 1）或太大（数百万个线程）。这些值的微调需要在一个尽可能接近真实环境的环境中进行测试，但代码审查中可以很容易识别的一个共同问题是当资源池（例如线程池或连接池）真正太大时。逻辑表明越大越好，但当然每个这样的对象都占用资源，管理数千个它们的开销通常比保持它们可用带来的好处高得多。如果有疑问，默认设置通常是好的起点。偏离默认设置的代码应该通过某种测试或计算来证明其价值。

审阅者可以轻松识别的警告信号

一些类型的代码立即表明可能存在性能问题。这将取决于使用的语言和库（请在评论中告诉我们您环境中“代码异味”的情况）。

反射

在 Java 中，使用反射比不使用反射要慢 15。如果你正在审查包含反射的代码，询问是否绝对需要这样。

15 http://docs.oracle.com/javase/tutorial/reflect/index.html

反射

上面的截图显示了一个审阅者在 Upsource 中点击一个方法来检查它的来源，你可以看到这个方法是返回了 java.lang.reflect 包中的某个东西，这应该是一个警告信号。

超时

当你审查代码时，你可能不知道操作的正确超时时间是什么，但你应该思考“当这个超时正在计时下降时，对整个系统的影响是什么？”。作为审阅者，你应该考虑最坏的情况——在 5 分钟的超时计时过程中应用程序是否被阻塞？如果将这个设置为 1 秒钟，最坏会发生什么？如果代码的作者不能证明超时长度的合理性，而你，作为审查者，不知道选定值的利弊，那么这是一个邀请了解这些影响的某人参与的好时机。不要等到你的用户告诉你性能问题时才行动。

并行

代码是否使用多个线程来执行一个简单的操作？这会增加更多时间和复杂性而不是提高性能吗？在 Java 8 中，这可能比显式创建新线程更微妙：代码是否使用了 Java 8 的新颖并行流但没有从并行性中获得好处？例如，在一个小数目的元素上或在对一个执行非常简单操作的流上使用并行流可能比在一个顺序流上执行操作要慢。

并行

在上面的代码中，添加并行的使用不太可能给我们带来任何好处——这个流正在作用于推文，因此是 140 个字符的字符串。对一个将要处理这么少单词的操作进行并行化，可能不会提高性能，并且将操作分散到多个并行线程的成本几乎肯定高于任何收益。

正确性

这些事情并不一定会影响您系统的性能，但它们在很大程度上与在多线程环境中运行有关，因此与主题相关。

代码是否使用适用于多线程环境的数据结构？

线程安全

在上面的代码中，作者在第 12 行使用 ArrayList 存储所有会话。然而，这个代码，特别是 onOpen 方法，是由多个线程调用的，所以 sessions 字段需要是一个线程安全的集合结构。对于这种情况，我们有几种选项：我们可以使用 Vector16，我们可以使用 Collections.synchronizedList()17 来创建一个线程安全的 List，但最合适的选项可能是使用 CopyOnWriteArrayList18，因为列表变化的频率远低于它被读取的频率。

线程安全

17http://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#synchronizedList-java.util.List-

18http://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CopyOnWriteArrayList.html

代码是否容易受到竞争条件的影响？

编写在多线程环境中使用时可能引起微妙竞争条件的代码是相当简单的。例如：

竞争条件

尽管增量代码只在一行（第 16 行），但在另一个线程在这段代码获取它和设置新值之间，仍然有可能增加 orders。作为审阅者，留意那些非原子的 get 和 set 组合 19。

代码是否正确使用锁？

与竞争条件相关，作为审阅者你应该检查被审查的代码没有允许多个线程以可能冲突的方式修改值。代码可能需要同步 20、锁 21 或原子变量 22 来控制代码块的更改。

性能测试对于代码是否有价值？

很容易写出差的微基准测试 23，例如。或者如果测试使用与生产数据完全不类似的数据，它可能会给出误导性的结果。

缓存

虽然缓存可能是防止过多外部请求的一种方式，但它也带来了自己的挑战。如果被审查的代码使用了缓存，你应该寻找一些常见的问题，例如，不正确地失效缓存项。

代码级别的优化

如果你正在审查代码，而你是一名开发人员，下面的部分可能有优化建议是你希望提出的。作为一个团队，你需要提前了解性能对你们有多重要，以及这些类型的优化是否对你的代码有好处。

对于大多数不是构建低延迟应用的机构来说，这些优化可能属于过早优化的 24 范畴。

• 代码是否不需要同步/锁时也使用了？如果代码总是运行在单个线程上，锁是不必要的开销。
• 代码是否在没有必要时使用线程安全的集合结构？例如，Vector 可以被 ArrayList 替换吗？
• 代码是否使用性能较差的数据结构来执行动作？例如，使用链表但需要定期在其中搜索单项。
• 代码是否使用锁或同步时可以使用原子变量代替？
• 代码能否从延迟加载中受益？
• if 语句或其他逻辑能否通过将最快的评估放在首位来短路？
• 代码中是否有大量字符串格式化？这能否更高效？
• 日志语句是否使用了字符串格式化？它们是否被一个检查日志级别的 if 保护，或者使用一个求值懒惰的供应商？

日志

上面的代码只有在记录器设置为 FINEST 时才会记录消息。然而，昂贵的字符串格式会每次都发生，不管消息实际上是否被记录。

日志

通过确保只在这个代码在日志级别设置到会将消息写入日志的价值上运行来提高性能，如上面的代码所示。

Logging

24 http://www.oracle.com/technetwork/articles/java/architect-evans-pt1-2266278.html

在 Java 8 中，这些性能收益可以通过使用 lambda 表达式来实现，而无需样板代码。由于 lambda 的使用方式，只有在消息实际上被记录时才会进行字符串格式化。这应该是 Java 8 中首选的方法。

总结

正如我在原始列表中所列出的审查时要寻找的事项一样，本章突出了你的团队在审查期间可能希望一贯检查的一些区域。这将取决于你项目的性能要求。

尽管这个章节针对所有开发人员，但许多例子都是 Java / JVM 特定的。我想以一些简单的建议结束，供 Java 代码的审阅者查找，这将为 JVM 优化你的代码提供良好的机会，这样你就不用：

• 编写小的方法和类
• 保持逻辑简单——没有深层嵌套的 if 或循环

对人类来说越容易理解的代码，JIT 编译器 25 就越有可能理解你的代码到足以优化的程度。这应该在代码审查期间很容易看出——如果代码看起来可理解和干净，它也有很好的机会表现良好。

当谈到性能时，要了解有些领域你可能能够在代码审查期间得到快速胜利（例如，不必要的数据库调用） that can be identified during code review, and some areas that will be tempting to comment on (like the code-level optimisations) that might not gain enough value for the needs of your system.

25 http://www.oracle.com/technetwork/articles/java/architect-evans-pt1-2266278.html