互联网上有大量有关代码审查的信息：
- 对他人代码的审查如何影响公司的文化
- 监管安全检查
- 简明手册
- 一长串建议
- 人际关系视角的代码审查
- 为什么我需要代码审查
- 可靠的方法
- 有关经过验证的方法的更多信息
- 统计信息：多少代码审查有助于捕获错误
- 代码检查期间发生的错误的示例
是的，当然有关于此主题的
书籍 。 简而言之，本文描述了Palantir如何组织代码审查。 在那些文化不接受同行评议的组织中，首先阅读Karl E. Wiegers的精彩文章《
人脸的代码修订
》，然后尝试遵循本指南可能会很有用。
本文是
基于与
Baseline （我们的Java代码质量控制工具）合作而
提出的质量改进建议 。 它涵盖以下主题：
- 为什么，什么以及何时尝试实现代码审查
- 准备代码进行审查
- 代码审查执行
- 代码审查示例
翻译成Alconost 
XKCD漫画“代码质量”，根据CC BY-NC 2.5许可复制动机
我们从事代码审查（RC）以提高代码质量，因此希望对
团队和企业文化产生
积极影响。 例如：
- 该代码的作者很受激励，因为他们知道审核小组将考虑他们的更改请求：作者试图清理粗糙之处，遵循操作计划并总体上改善整个提交工作。 当同事们承认您在编写代码方面的专业水平时-对于许多程序员来说，这是一个为自己感到骄傲的机会。
- 共享知识可以通过多种方式帮助开发团队：
-在RK之下，清楚地报告了已添加/更改/删除了功能的哪一部分，以便将来团队成员可以依靠已经完成的工作。
-代码的作者可以使用审阅者自己将学到的技术或算法。 总体而言，代码审查有助于
提高整个组织的质量标准 。
-审稿人可能对编程技术或代码库本身有所了解，这将有助于改进或巩固所做的更改； 例如，某人可以同时开发或修改完全相同的机会。
-积极的联系和沟通加强了团队成员之间的社会联系。
- 由于代码库的一致性 ，因此代码本身更易于阅读和理解。 防止bug并促进定居和游牧的程序员之间的协作更加容易。
- 该代码的作者很难判断自己创作的可读性 ，而对于审阅者来说，这很容易，因为他看不到该代码位于什么上下文中。 人类可读的代码更易于重用，错误更少，并且更持久。
- 挑剔的审阅者从侧面查看代码时，通常更容易发现随机错误 （例如拼写错误）以及结构错误 （例如不可执行的代码，逻辑或算法错误，性能和体系结构问题）。 根据研究，即使是简短的非正式代码审查也会显着影响代码的质量和错误的发生 。
- 合规性要求和法规框架通常需要强制性审查。 代码审查是避免常见安全问题的好方法。 如果增加了在这种环境中或与此机会相关的安全性要求，则将由监管机构的管理人员推荐（甚至要求）进行审查（ OWASP指南就是一个很好的例子）。
寻找什么
这个问题没有明确的答案，每个开发团队都需要就自己的方法达成一致。 一些团队更喜欢检查对主开发分支所做的任何更改，而其他团队则考虑“琐碎的门槛”，超过此门槛就必须进行验证。 在这种情况下，您必须在有效利用程序员的时间（作者和代码审阅者）与保持代码质量之间取得平衡。 在某些严格的上下文中，有时需要检查代码中的所有内容，即使是最琐碎的更改。
每个人的代码审查规则都相同：即使您是团队中最高级的开发人员，这也不意味着您的代码不需要审查。 即使代码（有时会发生）是完美的，但审查也会提供指导和合作的机会，并且至少有助于从不同的角度审视代码库。
什么时候检查
在成功完成自动检查（测试，设计，其他连续集成阶段）之后，但在将新代码与主存储库分支合并之前，应进行代码检查。 通常，我们不求助于自上次发布以来对代码进行的全部更改的正式验证。
对于应该作为单个单元添加到代码主分支中的复杂更改，但是这些更改过于广泛以至于它们实际上不能在一次检查中涵盖，请尝试分阶段进行检查。 我们创建主要功能部件/大功能部件分支和许多辅助功能部件（例如feature / big-feature-api，feature / big-feature-testing等），每个部件都封装了通用功能的子集，每个这样的分支部件根据功能/大功能的主要分支进行检查。 在所有辅助分支都合并为feature / big-feature之后，创建代码审查以将后者注入主分支。
准备代码进行审查
该代码的作者有义务以一种可消化的形式提供该评论的代码，以免花费额外的时间来使评论者不致动摇：
- 范围和大小 。 变更应与狭窄的，定义明确的，自给自足的范围有关，并应在审查中完全涵盖。 例如，更改可能与某些功能或错误修复的实现有关。 短暂的更改比长的更改更可取。 如果评论所包含的材料包含超过5个文件的更改，或者记录需要超过1-2天，或者评论本身可能需要20多分钟，请尝试将材料分解为几个独立的片段，并分别进行检查。 例如，开发人员可以提交一个更改，该更改根据接口和文档定义了新功能的API，然后添加了第二个更改，这些更改描述了这些接口的实现。
- 您只需要发送完整，经过独立测试 （使用diff）和经过独立测试的材料。 为了节省审阅者的时间，请测试提交的更改（即运行测试套件），并确保代码在本地和连续集成服务器上通过所有程序集以及所有测试和质量控制的所有阶段， 然后选择审稿人 。
- 重构更改不应影响行为，反之亦然； 影响行为的更改不应涉及重构或重新格式化代码。 这样做的原因有很多：
-与重构相关的更改通常会影响许多文件中的许多代码行-因此，将
不仔细检查此代码。 计划外的行为更改可能会泄漏到代码库中，甚至没人会注意到它。
-与重构相关的重大更改违反了与源代码管理相关的移动，选择和其他“魔术”机制。 撤消覆盖整个存储库的重构完成后所做的这种行为更改非常困难。
-人们花在代码审查上的昂贵工作时间应该去检查程序逻辑，而不要争论代码的样式，语法或格式。 我们更喜欢使用自动化工具来解决此类问题，尤其是
Checkstyle ，
TSLint ，
Baseline ，
Prettier等。
提交讯息
以下是有效提交消息的示例，该消息是根据广泛使用的标准设计的：
( 80 ) , , . 72 . , — . , ( ); , rebase, . - : « », « » « ». -, , , git merge git revert. . .
尝试描述提交期间所做的更改以及这些更改的精确程度：
> . . . > . jcsv- IntelliJ
搜索评论者
通常，提交的作者搜索一位或两位熟悉代码库的审阅者。 通常以这种身份担任项目经理或高级工程师。 建议项目所有者订阅自己的项目，以便接收有关新代码审查的通知。 在三个或更多审阅者的参与下，代码审阅通常被证明是无用的或适得其反的，因为不同的审阅者可能会提出相互矛盾的变更。 这可能表明对正确实施存在根本分歧，并且不应在代码审查期间解决此类问题，而应在所有相关方亲自参加或以视频会议方式参加的扩展会议上解决。
代码审查执行
代码审查是不同团队成员之间的同步点，因此有可能停止工作。 因此，代码审查应该是可操作的（大约要花几个小时，而不是几天），并且团队成员和领导者应该知道要花多少时间来检查和优先安排分配给它的时间。 如果您觉得您没有时间按时完成审阅，请立即将提交的情况告知作者，以便他为您找到替代方案。
审查应该相当彻底，以便审查者可以向其他开发人员详细解释变更。 因此，我们将确保至少两个（而不是一个）知道代码库的详细信息。
作为审阅者，您必须遵守代码质量标准并将其保持在较高水平。 代码审查不是一门科学，而是一门艺术。 这只能在实践中学习。 有经验的审阅者应首先尝试让经验不足的同事进行更改，并让他们成为第一个进行审阅的同事。 如果作者遵循上述规则（尤其是与自测和初步代码执行相关的规则），那么审阅者在审阅以下内容时应注意：
目的
- 代码是否达到了作者设定的目标？ 任何更改都必须具有特定的原因（新功能，重构，错误修复等）。 提议的代码是否真的将我们引向了这一目标？
- 提出问题。 函数和类必须合理。 如果不清楚他们对审阅者的分配，则可能意味着该代码应重写或由注释或测试支持。
实作
- 想想如何解决这个问题。 如果没有，那为什么呢？ 您的代码可以处理更多（边界）情况吗？ 也许它更短/更容易/更清洁/更快/更安全，并且在功能上还没有恶化？ 也许您捕获了一些现有代码未涵盖的深层规律性？
- 您看到创建有用的抽象的潜力吗？ 如果代码是部分重复的，则通常意味着可以从中提取该功能的更抽象或更通用的元素，然后可以在其他上下文中重用。
- 像对手一样思考，但是要保持友善。 尝试通过抛出有问题的配置/输入数据来破坏他们的代码，从而“抓住”那些走捷径或错过特定案例的作者。
- 考虑一下库或现成的工作代码。 如果某人重新实现了现有功能-这不仅是因为他不知道现成的解决方案是否存在。 有时，有意重新设计了代码或功能-例如，以避免依赖。 在这种情况下，应在代码中对此进行明确注释。 现有库中是否已提供此功能？
- 更改是否符合标准模式？ 在现有的代码库中，经常会跟踪与命名约定，程序逻辑分解，数据类型的定义等有关的规则。 通常希望根据现有模式实施所有更改。
- 是否在更改期间添加了在编译期间或运行时（尤其是子项目之间）发生的依赖关系？ 我们努力争取产品代码的弱一致性，也就是说，我们尝试使相关性降到最低。 与依赖关系和构建系统有关的更改应特别仔细地检查。
可读性和风格
- 考虑一下您如何阅读此代码。 您能否足够快地掌握他的概念？ 是否合理布局，是否易于跟踪变量和方法的名称？ 我可以在多个文件或功能上跟踪代码吗？ 您是否曾经被某个地方的命名不一致所困扰？
- 该代码是否符合著名的编程和样式准则？ 代码是否按样式，API命名约定等从整个项目中分解出来？ 如上所述，我们尝试使用自动工具解决样式方面的纠纷。
- 代码中是否包含任务列表（TODO）？ 任务列表只是在代码中累积，最终变成镇流器。 分配作者向GitHub Issues或JIRA提交票证，并将任务编号附加到TODO。 建议的更改中不应包含任何注释代码。
可用性支持
- 阅读测试。 如果代码中没有测试，但应该存在，请要求作者写一些。 真正未经测试的功能很少见，而未经测试的功能实现却很少-不幸的是，常常如此。 自己检查测试：它们涵盖有趣的情况吗？ 读起来方便吗？ 此项测试后，总体测试覆盖率是否降低？ 考虑一下此代码可能如何失败。 测试设计标准和核心代码通常不同，但是测试标准也很重要。
- 对该片段进行复查是否存在违反测试代码，侵入代码或集成测试的风险？ 通常，在提交/合并之前不会进行此类检查，但是如果忽略了这种情况，那么每个人都会受苦。 请注意以下事项：删除测试实用程序或模式，更改配置，更改工件的布局/结构。
- 此更改是否违反向后兼容性？ 如果是这样，是否可以在此阶段对发行版进行此更改，还是应该推迟到以后的发行版？ 违规行为可能包括对数据库或其架构的更改，对公共API的更改，对用户级别任务流的更改等。
- 此代码是否需要集成测试？ 有时，仅使用单元测试就无法正确检查代码，尤其是当它与外部系统或配置交互时。
- 在文档中留下有关此代码，注释和提交消息的反馈。 大量注释会阻塞代码，并且意味着提交消息会使以后不得不使用代码的人感到困惑。 并非总是如此，但是随着时间的流逝，高质量的评论和提交消息一定会很好地为您服务。 （记住，当您看到华丽或糟糕的评论或提交消息时。）
- 外部文档是否已更新？ 如果为您的项目维护了README，CHANGELOG或其他文档，是否进行了更新以反映所做的更改？ 过时的文档可能比根本上没有危害更大，并且在将来修复错误比现在进行更改更昂贵。
不要忘了赞扬简洁/可读/高效/漂亮的代码。 相反，拒绝或拒绝提议进行审查的代码并不粗鲁。 如果更改过多或不重要-拒绝更改，并说明原因。 如果由于一个或多个致命错误而使更改看起来不可接受，请再次有理由拒绝该更改。 有时，代码审查的最佳结果是说：“让我们完全不同地做”或“让我们根本不做”。
尊重同行评审。 尽管对手的立场是正确的，但您没有意识到这个机会，也无法为他决定一切。 如果您无法获得有关该代码的同行评审意见，请安排实时咨询并听取第三者的意见。
安全性
确保所有API端点均已按照其余代码库中采用的规则进行了充分的授权和认证。 查找其他常见缺陷，例如：配置不足，恶意用户输入，缺少日志条目等。 如有疑问，请向安全专业人员显示经过同行评审的代码。
评论：简洁，礼貌，激励
审查应简明扼要，保持中立。 批评代码，而不是作者。 如果有不清楚的地方-要求澄清，不要以为整件事都在于作者的无知。 避免所有格代词，尤其是在价值判断上：“
我的代码在
您进行更改之前就起作用”，“
您的方法中存在错误”等。 不要砍断肩膀：“
根本不起作用”，“结果
总是不正确的。”
尝试区分建议（例如“建议：提取方法，这将使代码更清晰”），必要的更改（例如“ Add
Override ”）和需要澄清或讨论的观点（例如“此行为是否正确？”）。 ？如果是，请添加注释以解释其逻辑。”） 尝试将链接或指针指向问题的详细描述。
完成代码审查后，请指出对作者的评论的详细程度，进行更改后是否要重复审查，例如：“在进行了较小的更正后，您可以安全地将代码上传到主分支”或“请注意我的评论，让我知道何时可以再次看到该代码。”
评论回应
特别是，代码审查旨在改善作者提出的变更请求； 因此，即使您不同意推荐人的建议，也不要对其怀有敌意，要认真对待。 回应任何评论，甚至是通常的“ ACK”（已批准）或“ done”（完成）。 说明为什么做出这个或那个决定，为什么在代码中具有某些功能，等等。 如果您无法与审阅者达成共识，请实时安排咨询并听取第三方专家的意见。
更正应固定在同一分支中，但应在单独的提交中进行。 如果您在审核阶段将提交放在一起，那么审核者将很难跟踪更改。
不同团队的代码合并策略不同。 在某些公司中，代码合并仅由项目所有者信任；在其他公司中，参与者可以在其代码经过审查和批准后才能执行此操作。
Tête-à-tête代码审查
通常，类似异步diff的工具（例如Reviewable，Gerrit或GitHub）非常适合代码审查。 如果我们谈论的是复杂的评论或评论，而参与者的经验或专业水平相差很大，则亲自进行评论会更有效-既可以坐在一台显示器或投影仪前，也可以通过视频会议或屏幕共享工具远程进行。
例子
在以下示例中，使用以下方式输入列表中的审阅者评论
命名不一致
class MyClass { private int countTotalPageVisits;
方法签名不一致
interface MyInterface { public Optional<String> extractString(String s);
图书馆使用
//R: MapJoiner Guava String joinAndConcatenate(Map<String, String> map, String keyValueSeparator, String keySeparator);
个人喜好
int dayCount;
虫子
//R: numIterations+1 – ? // – numIterations? for (int i = 0; i <= numIterations; ++i) { ... }
建筑考虑
otherService.call();
关于翻译这篇文章由Alconost翻译。
Alconost以70种语言
本地化游戏 ，
应用程序和网站 。 母语翻译，语言测试，带有API的云平台，持续本地化，24/7项目经理，任何格式的字符串资源。
我们还制作
广告和培训视频 -适用于销售，图像，广告，培训，预告片，专家，Google Play和App Store的预告片的网站。
更多细节