代码审查不是战场,审查员也不是作者的对手。他们的目标是一致的——解决产品问题并创建高质量的代码库。让我们深入探讨并了解如何从审查者的角度进行一次代码审查。
不要浪费时间
总有些问题时常重复出现。先是在一个拉取请求中,然后又在另一个拉取请求中;先是来自一个作者,然后又来自另一个作者。这些问题完全相同,这就是例行公事。事实上,如果某件事情可以自动化,那么它就必须自动化。
代码风格。没有必要为代码风格而争论不休,因为早在几十年前,项目中的每个人或整个社区就已经对代码风格进行了多次定义。在 linter(代码检查工具) 和 formatter(格式化工具) 中设置字符串的长度、方法和类的名称,然后忘掉它吧。
测试。要将所有测试(单元测试、集成测试、端到端测试)及其启动、覆盖范围等相关事项实现自动化。只需进行设置,并为指标设定一个可接受的阈值,例如,在 PR(拉取请求)中的新代码覆盖率不应低于 90%,这种简单的设置能让很多人的工作更轻松。
不要自我重复(DRY)。应该将常见的重复内容集中到一个地方,以便对其进行简单、集中的更改或修复。收集所有应用程序的代码重复百分比,并以相同的方式进行测试。
代码分析。代码分析有助于收集更多数据和指标。它不仅会检查审查中的代码,还会检查如何将其集成到现有生态系统中。一些分析工具会根据历史案例提供可能存在的漏洞或安全热点报告。将存储库与代码分析工具集成,并在每次代码审查时运行这些工具。
总结。工程师应该专注于解决问题,而不是重复常规。我可以保证的是,如果能将上述任何事件至少基本自动化,代码审查的平均质量就会大幅提高。这能为审查人员节省时间。如果出现以下情况,就需要检查代码?
- 并非所有测试都通过
- 测试覆盖率不足/低于公认的百分比
- 代码重复率高于可接受水平
- 代码有异味
- 意外的安全热点
通用规则
尊重。礼貌待人,尊重作者。请记住,代码审查的参与者是来互相帮助的,他们有着共同的目标。可以批评代码,但绝不能批评人。
任务。在完全理解问题所在之前,不要进行代码审查。获取有关问题所在、实际案例、条件、重现步骤、功能受众等信息。要求进行代码演示、代码评审会议,甚至只是同步会议,以明确特定任务的各个方面。
建议。当提出任何改变的建议时,解释原因。提供背景、示例、细节和信息,并分享有关资源的链接——为什么作者应该进行更新。一个词或一行话的评论有时真的很难理解。请记住,通常任务可能有多个解决方案,因此在建议更改之前,请尝试理解选择此解决方案的确切原因。使用提交历史,以及带有良好消息的结构化提交可以提供理解的关键。
审查流程
下面我收集了一些真正值得关注的基本类别和问题,按照优先级从最重要的开始排序。
业务目标
深入研究,不仅要研究代码,还要研究其背后的业务逻辑。首先,任何代码都必须执行指定的任务并达到指定的目标。不管代码有多好,不管它写得有多好,如果它不能实现它的目标,它就是无用的。代码不是为代码而写的。编写代码是为了添加新功能,开发和推动产品向前发展。不要将检查限制在快乐路径上,要考虑边缘情况以及如何处理它们。所有这些都可以概括为这个问题——它能解决问题吗?
实现
接下来,开始关注数字、指标和报告。从不同角度分析代码。
安全性。它带来了漏洞还是解决了漏洞?在受到攻击时它会有多稳定?被动还是主动?比如分布式拒绝服务攻击(DDoS)或者任何类型的注入(如 SQL 注入、跨站脚本等)?
错误处理。如何正确处理错误?应用程序会崩溃或向错误跟踪软件发送报告吗?它会向最终用户显示所有堆栈跟踪吗?它是可恢复的失败操作吗?数据会被损坏或碰撞吗?
性能。新更改后性能是否受到影响?该代码可能导致内存泄漏?优化有多好?是否做了所有的事情来使代码高效(缓存系统、资源池、数据压缩等)?
集成。它如何与其他模块和系统协同工作?是否能提高代码的一致性?能否方便地与其他实现或集成进行交换?代码与系统其他部分其他版本的兼容性如何(如新版本的向后兼容性)?
日志和跟踪。这可以简化调试和故障排除过程,也可以使其变得更加复杂。例如,记录集成的第三方服务的响应时间有助于识别停机时间。日志和跟踪是否足够?多还是少?
仔细研究并回答这些问题,就能确定实施工具的正确选择。
可维护性
这里的一个主要问题是——“没有作者的代码如何生存?”
可读性。代码如同构成单词或者句子的字母,所有的代码组成完成后,就如同在阅读一本书籍,不过这本“书籍”是用一种特定的语言写的:编程语言。所以可读性应该从字面上理解,代码应该用写得好的字符(如参数、变量等)构建一个故事(如类、函数),它们应该采取行动(调用其他函数、变异或不可变等)。
值得关注的问题:该代码的可读性如何?它可以由作者以外的人来维护吗?命名参数、变量、函数等的可理解性如何等等。
文档。在开发过程中,文档可以节省大量时间,减少同步时间,简化入职流程,总之是项目知识库的良好存储。
值得关注的问题:代码文档的质量如何?阅读后是否会留下疑问?所有需要的 MD 文件和外部文档是否会根据变更进行添加/更新?
可重用性。为防止代码重复,如果多个模块的逻辑是共通的,就可将其移至助手、实用程序等共享位置。
值得关注的问题:代码的某些部分能否在其他地方重复使用?如果不能,其独特性是否合理?如果是,是否为达到这一指标而进行了过度设计?
设计。代码不应该重新发明轮子。在社区中已经有许多被认可的最佳实践和定义好的设计模式,它们是软件工程中常见问题的解决方案。
值得关注的问题:代码是否采用了最佳实践和模式?是否以正确的方式使用?
印象。代码应当激励以某种方式与它现在或未来产生交集的任何人,努力做到同样出色和高质量,甚至更好。
值得关注的问题:在合并之后,代码库是否变得更好?其他工程师会对使用这段代码感到兴奋吗?
代码审查:成长的机会
做好代码审查是一项艰巨的工作。审核员是第一道技术质量关。在合并之前,代码归作者所有并由其管理,但合并之后,责任将移交给整个团队。这就是为什么审查员应该关注代码的稳定、可靠和无懈可击。代码审查是一次成长的机会。对审阅者和作者来说都是成长。