-
2021-11-08 23:52:12
代码评审的流程有以下两种:
-
提交前评审(pre-push code review)
-
提交后评审(post-push code review)
提交前评审的流程
-
程序员在试图提交代码变更到代码库之前,先提交变更申请,变更申请包含了这次变更的内容,评审人
-
评审人查看变更内容,评估变更,与变更申请人沟通,评估是否通过变更;
-
如果评审人通过变更,则变更申请人才可以提交代码到代码库;
-
如果评审人不通过变更,则变更申请人需要根据讨论结果或评审建议做出修改,直到与评审人达成一致,通过评审,才可以提交代码到代码库
提交后评审的流程
-
程序员提交变更代码到代码库;
-
评审人审查这次变更的内容,如果评审通过,则标记此次的变更已审查;
-
如果评审人有疑义,则与变更人
更多相关内容 -
-
代码评审系统ReviewBoard和Gerrit
2021-01-30 07:03:57不论是公司里还是公司外,正经的多人合作开发最好要经常做代码评审,其必要性不用我多说,但是如何做代码评审确实个头大的事情,我个人是非常反对拉一票人去会议室开投影仪一行行讲的,太浪费资源和时间了。... -
C++代码评审检查表.xls
2021-07-16 10:12:39C++代码评审检查表:包含常用C++代码审查内容,以excel表格形式列出,详细清晰 -
代码评审检查表.xls
2019-11-09 16:41:24Code Review是一种用来确认方案设计和代码实现的质量保证机制,通过这个机制我们可以对代码、测试过程和注释进行检查。...代码评审检查文档,统一规范文档、适合规范制定,以及做代码参考文档 便于学习。 -
java代码评审检查表.xls
2021-07-16 10:11:46java代码评审检查表:包含java常用代码审查内容 -
代码评审表——Java
2021-01-21 04:03:48Java代码评审检查表JAVA代码评审表约定:总分100=基础项50分+重点项50分。附加分上限10分。针对每一大项评分,细项作为评分的参考依据。大项评分小于该项总分的60%或大于该项总分的80%必须 -
代码评审这点事,元芳你怎么看
2021-03-02 08:26:11百度百科上说:“代码评审也称代码复查,是指通过阅读代码来检查源代码与编码标准的符合性以及代码质量的活动。”这篇百科的内容好像是几年前CSDN上的一篇博文,但不管他们怎么抄,代码评审大概就是这么个意思。代码... -
软件代码评审表
2019-01-25 17:19:13该表格规定软件代码评审表格式,明确了各项评审内容,避免遗漏。 -
静态测试之代码评审的一些建议
2021-03-23 14:26:31静态测试之代码评审的一些建议!Facebook产品经理王准的一些建议:作为审查者,一定要读懂diff;所有被接受的diff必须是在读懂的前提下。做审查者的人要有“将来如果这些代码线上出问题,我要帮助支持”的心理准备。 ... -
word源码java-Code-standards-and-specifications:代码评审和规范
2021-06-05 22:51:29根据自己的编程习惯和缺陷,建立自己的代码规范和代码评审标准,并且今后务必加以使用; 需要提交代码规范和代码评审标准两个表格,使用Word即可。 代码规范 实现及注意事项 一.实验内容 这个规范和标准主要根据从... -
代码评审会流程和评审标准
2018-12-27 20:51:39代码评审会议流程+代码评审标准+代码评审打分表模板,按需下载。 -
11个高效的同行代码评审最佳实践
2021-03-23 14:07:4311个高效的同行代码评审最佳实践。这11项针对轻量级高效同行代码评审最佳实践被证明是有效的,它们建立在一个通过结合使用IBMRationalTeamConcert与SmartBearCodeCollaborator对Cisco系统的开发进行案例研究的基础之... -
代码评审一般检查表.xls
2021-07-16 10:10:24代码评审一般检查表:包含常用代码评审内容 -
Bitbucket都代码评审「Bitbucket code review」-crx插件
2021-03-10 01:42:06对Bitbucket和JIRA的改进,用于更简单的代码审查。 Bitbucket和JIRA中用于提交和部分提交的标记为已读功能。 -在Bitbucket和JIRA中将提交标记为已读/未读-在bitbucket提交页面中隐藏已查看的文件差异-忽略指定作者的... -
代码评审规范.pptx
2019-08-10 14:38:10代码评审规范指南,仅供参考,如有雷同,纯属巧合 欢迎下载 -
C++代码评审(CodeReview)等级标准
2021-01-30 07:34:14C++代码评审(CodeReview)等级标准 -
测试左移之代码评审
2021-02-25 11:21:39多数项目中,代码评审工作是由开发同事相互执行的。但往往开发同事为了赶进度,并没有时间进行代码评审,导致很多明显的Bug被遗留到了测试阶段。那代码评审是否可以由测试人员来做呢?显然是可以的。诚然多数测试... -
代码静态测试:高效同行代码评审最佳实践
2021-03-23 14:30:52摘要:这11项针对轻量级高效同行代码评审最佳实践被证明是有效的,它们建立在一个通过结合使用IBM:registered:RationalTeamConcert:trade_mark:与SmartBearCodeCollaborator对Cisco系统的开发进行案例研究的基础之上... -
代码评审的最佳解决方案
2021-09-01 09:24:48代码评审可降低故障率,代码评审是结对编程相互切磋相互学习的方式,是敏捷开发模式中的一个重要环节,是保障代码质量的重要手段。云效代码管理 Codeup,10万企业都在用的代码管理平台,提供代码托管、代码评审、...代码评审可降低故障率,代码评审是结对编程相互切磋相互学习的方式,是敏捷开发模式中的一个重要环节,是保障代码质量的重要手段。云效代码管理 Codeup,10万企业都在用的代码管理平台,提供代码托管、代码评审、代码扫描、质量检测、持续集成等功能。
背景
在行业激烈竞争业务快速运转的今天,如何在实现快速交付的同时保证代码质量一直以来都是技术团队反复探讨的话题之一。
代码质量可以通过两个维度来度量:一是代码的缺陷情况,二是代码可读性。代码缺陷小则引发线上故障影响业务正常运行,大则可能造成企业重大经济损失甚至信用受损,重则引发社会动荡;而代码可读性也尤为重要,可读性差则维护成本高,修改相关模块代码无异于埋雷,一不小心就会炸。Google 最早引入 CodeReview 的初衷就是为了保证代码具有良好的可读性(“Force developers to write code that other developers couldunderstand”),并将 Readability Review 延用至今。
有效的代码评审可降低故障率,本篇我们重点介绍团队如何在云效上做好代码评审。
用户的诉求或问题
- 没有统一的流程管控,团队同学基本不做评审,质量无法得到保障
- 作为开发者在创建代码评审时,不清楚改动应指派哪些评审人
- 如何做好代码评审
云效代码评审解决方案
如何设置卡点
保护分支允许管理员根据团队的 workflow ,为单个分支或分支规则建立合适、灵活的分支管控,如:发布分支不允许所有人 push,必须通过代码评审才能 merge,以及一些 merge 的卡点条件。合并检查与分支权限协同管控,能为团队提供更加灵活可控的开发工作流程。设置后该代码库下创建目标分支符合改保护分支规则的合并请求,均走该卡点配置。
说明 :
立即体验:云效代码分支设置1、要求合并前通过代码评审
可设置人工评审卡点,如评审最少通过人数、库内什么角色成员能通过等。
2、要求合并前通过自动化执行检查提供官方插件 Java 代码规约扫扫描和敏感信息检测,且支持卡点设置。具体参见:链接文档
评审人选择
作为开发者进行代码提交后创建代码评审,当代码库较大参与开发同学较多时,不知道该指派哪几位同学作为本次改动的 reviewer。可采用以下几种方式:
1、保护分支默认评审人
不同研发模式,不同分支可能存在不同的负责人。代码库管理员可以通过将分支负责人配置为保护分支默认评审人,达到创建即指派分支负责人的效果。如:交付团队存在基线版本,交付不同定制方,每个定制版本均是一个分支形态,均存在相应版本负责人;
2、CodeOwner 模式理想情况下的 Code Review,是评审人员就是最熟悉这块代码的同学,但是实际上Author并不一定知道谁应该 review 自己的改动。CodeOwner 机制就是去维护一个文件,指明哪些路径下的哪些文件被谁 own 应该谁去 review,当 push 更改中包含这些文件时,就会将 own 这些代码的人自动加到评审人中。
我们使用了一个 CODEOWNERS 文件来记录代码库中各模块或文件的负责人,该文件应位于分支的根目录下。当一次评审发起后,并且代码库启用了 CodeOwner 审核,那么系统会在评审目标分支的根目录下,寻找 CODEOWNERS 文件,并从其中读取相关设置。当文件与CodeOwner 出现了 1:N 的情况时,例如一个文件对应了 A、B、C 三位 Owner,此时只要有一位 CodeOwner 审核通过即可。此外,评审创建时或评审分支更新后,系统会自动检测需要参与评审的 CodeOwner,并把他们自动加入审核人列表中。
CODEOWNERS 文件中,对于路径的定义采用了 Glob 的语法。路径规则追加空格后,采用的形式定义相关 Owner,username 需使用对应 Owner 已验证并绑定的邮箱。已绑定邮箱可在个人设置-个人信息查看。文件示例如下:
# 注释行,以下为配置正文,每一行代表一个配置。 # 一个路径规则后边,需要有一个或多个Owner # 用户 Tiger@alibaba.com,Dragon@alibaba.com 作为所有文件的CodeOwner ** @Tiger@alibaba.com @Dragon@alibaba.com # 用户 Alan@alibaba.com 作为所有java文件的CodeOwner **.java @Alan@alibaba.com # 用户 Ben@alibaba.com、Carl@alibaba.com 作为force-api目录下文件的CodeOwner force-api/** @Ben@alibaba.com @Carl@alibaba.com # 用户 Mike@alibaba.com 作为force-base/src/main/java目录下文件的CodeOwner force-base/src/main/java/** @Mike@alibaba.com
3、智能评审人推荐
即将上线,敬请期待。
代码评审最佳实践
以下为如何做好代码评审的最佳实践:
- 做好提交
将提交做小做好,写小提交就是将问题解耦:“Do one thing and do it well”。开源项目的提交通常都很小,每个提交只修改一个到几个文件,每次只修改几行到几十行。每个提交应该是一个完整的功能,可能是修改某个 bug 或完成某个小需求的开发,commit message 记录本次 commit 详细说明,大体分为:提交标题、主体 body、sign 签名,是阅读者能够清晰的理解改动的背景。
- 充分描述
对于代码评审描述应介绍本次改动的需求背景,改动范围及影响点。一段清晰的评审描述能让 reviewer 充分理解需求背景,快速开始评审,降低沟通成本。
- 小步快跑
在团队实践的过程中,经常碰到改动较大的评审,评审越大评审成本越高耗时越长。正确的方式是将 CodeReview 当做一个“日常习惯”而不是一个“盖章动作”。只有每次提交的内容尽可能的小而独立,才能真正让别人帮你 review。因此,我们不鼓励到临上线前才做 CodeReview,而是当拉出的分支做完一个小的提交后,就应该开始 CodeReview。如:新人入职完成了 API 的定义想让同学看下模型定义上是否存在问题,就可以采用以上方式。
对于开发中的代码评审,Author 可将代码评审的标题支持设置 WIP 标识 Work In Progress,使 Reviewer 有意识这不是一个完整的功能,且无法合并。
- 问题追踪
即使大家面对面坐着,讨论非常方便,事后仍要把评审中的问题记录进系统,进行问题沉淀,并由系统跟踪这些问题最终是否得到了解决,进行问题的跟踪和闭环。
- 定期度量
通过数据洞察获得团队质量情况,有策略的提升团队技术能力。关于代码库统计模块即将上线,敬请期待。
云效代码管理 Codeup,10万企业都在用的代码管理平台,提供代码托管、代码评审、代码扫描、质量检测、持续集成等功能,全方位保护企业代码资产,帮助企业实现安全、稳定、高效的代码托管和研发管理。
点击立即体验:云效代码管理 Codeup
关于我们
了解更多关于阿里云云效DevOps的最新动态,可微信搜索并关注【云效】公众号;
福利:公众号后台回复【指南】,可获得《阿里巴巴DevOps实践指南》&《10倍研发效能提升案例集》;
看完觉得对您有所帮助别忘记点赞、收藏和关注呦;
-
可视化代码评审工具 Phabricator.zip
2019-09-17 16:47:50可视化代码评审工具 Phabricator ,在代码审查(Code Review)方面,Facebook做了一个可视化的工具,现已开源,叫Phab... -
代码评审如何做好
2021-02-20 09:14:51Code Review 翻译成中文是代码评审,具体的定义可以看wiki。这篇 wiki 介绍说 Code Review 在帮助团队找到代码缺陷这件事上作用巨大:“代码审查一般可以找到及移除约65%的错误,最高可以到85%”。实际上, Code ...一、背景知识
1.1、关于
Code Review 翻译成中文是代码评审,具体的定义可以看 wiki。这篇 wiki 介绍说 Code Review 在帮助团队找到代码缺陷这件事上作用巨大:“代码审查一般可以找到及移除约65%的错误,最高可以到85%”。实际上, Code Review 的好处远不止这一条,它至少能在以下三个方面帮到我们:
-
传播知识。相信很多人第一次提交 Code Review 都有类似的经历:短短几百行代码,却被提了密密麻麻几十条 comments,更新了十多次代码,才最终被 accept 。其实当代码被 accept,提交代码的工程师通过这次 review 就学习到了代码规范和很多好的实践。同时,通过 review 更资深工程师的代码,年轻的工程师也更直观地学习架构和编码;另外,工程师之间也可以通过 review 代码来共享项目知识,看代码实现在绝大多数时候是了解项目的最好方式。
-
增进代码质量。这点也很容易理解,有经验的工程师可以在架构设计、代码细节等各个方面帮助到初学者。不同工程师也会有知识盲点,互相 review 进步也很快。另外,被 review 的代码质量更高还有一个很多人注意不到的心理因素:在状态不佳的时候,工程师难免会匆忙写些“潦草”的代码,但是当你知道自己的代码会被review 的工程师提交 comment 打回来,自然会更仔细些 : -)
-
找出潜在的 bug。将损失降至最低。这是大部分团队进行 Code Review 的目的。就像上面提到的,Code Review 在这方面效果不错。其实我认为大部分代码 bug 应该由单元测试,功能测试,性能测试和回归测试来保障。不过由于静态分析不理解业务,另外有些 bug 在测试中并不容易复现,这两种情况下,经验丰富的工程师来 review 代码就尤其重要了。
1.2、我是如何审代码的
1、首先我会关注代码风格规范是否和公司一致(可读性和可维护性),
2、然后关注代码调用是否破坏了当前代码架构,
3、其次是代码逻辑中的资源处理、线程、安全、异常处理,
4、再其次是业务实现逻辑,
5、最后是否有优化空间。
前面三点相对于参考checklist能够做到,部分处理也能够通过工具检查查出来,后面两点相对比较难,需要了解它的需求是什么样,需要了解这块的逻辑,然后才能够读懂逻辑实现是否有误。最后基于对于好的代码理解看看在扩展性和性能上是否有优化空间。
二、什么叫做好代码评审
那么我们怎样做好 Code Review 呢?两个方面:一是减负,Code Review 只做它应该做的事情。二是提高 Code Review 的效率。
2.1、如何提高效率
首先, 先列几个不该在Code Review时关注的
- Code review 不应该承担发现代码错误的职责。Code Review主要是审核代码的质量,如可读性,可维护性,以及程序的逻辑和对需求和设计的实现。代码中的bug和错误应该由单元测试,功能测试,性能测试,回归测试来保证的(其中主要是单元测试,因为那是最接近Bug,也是Bug没有扩散的地方)
- Code review 不应该成为保证代码风格和编码标准的手段。编码风格和代码规范都属于死的东西,每个程序员在把自己的代码提交团队Review的时候,代码就应该是符合规范的,这是默认值,属于每个人自己的事情,不应该交由团队来完成,否则只会浪费大家本来就不够的时间。
注意: 这里说在Code Review时不该关注, 并不是说不重要, 而是说, 这些部分应该有其他方式(例如自动化工具)去保障, 因为人的成本是最高的, 机器和程序才是便宜的。
Code Review 应该讨论的是功能实现、架构设计、代码质量。
1、检查代码风格和编程规范。
代码风格包括但不限于:命名规范、代码缩进、注释和文档等等
2、检查常规的 bad smell 和代码 bug
重复代码、过长函数等等
2.2、如何提高 Code Review 的效率
-
选用合适的工具。我用过 Phabricator、Gerrit、Gitlab 来 review 代码。这里推荐使用 Phabricator,就不过多介绍了,可以看 这里 的讨论。
-
每次只 Review 少量代码。新人经常犯的一个错误,花了两周甚至更多的时间写代码,然后又花了一些时间做测试,等他们自己觉得“大功告成”才敢自信地提交 review,殊不知,这个时候提交的 review 几乎没有什么意义。一方面,对于提交 review 的工程师来说,辛辛苦苦开发测试了两三周,就等 review 完成后 release 了,这时候如果收到各种需要修改代码的意见,心里难免会有抵触情绪,尤其是 review 的结果是架构需要改动,代码需要大面积重写,内心一定是崩溃的。另一方面,代码审查者如果面对数千甚至上万行代码,需要理清项目架构都需要花费大量时间,强行 review 这种代码,争论、修改、测试代码,很可能 一周甚至更长时间都完成不了 review,结果就是双方都痛苦不堪。在实践过程中,我们认为,频繁 review 代码并且每次只 review 少量代码非常重要,我自己认为 reivew 不超过 500 行代码比较好。
-
明确职责。 Review 过程中经常遇到的一个问题是 review 响应不及时。比如 assgin 给了工程师,工程师没有及时 Review,或者提交 review 的工程师没有及时响应修改意见。造成这种现象的原因大致有这么几种:工程师没有及时处理 review 的习惯;review 工程师需要项目的领域知识等。解决方法也很简单,Review 是项目开发的一部分,进度由开发工程师来负责,这样,Code Review 如果不顺利,应该由提交代码的工程师负责推动,如果需要讲解代码,提交代码的工程师应该主动走到 reviewer 工位上,说说背景知识和代码逻辑。也就是说,如果沟通受阻,工程师应该更积极的面对 reviewer ,毕竟大家是在花自己的时间来帮助他。
-
整理 checklist。如果你回顾犯过的编程错误就会发现,在某个阶段自己容易犯类似的错误。其实上,团队里的工程师也有这种情况。刚入门的工程师会出现 API 误用;刚加入团队的工程师对编码规范需要一段时间来适应;有些工程师在代码命名上总会犯同样的错误;也有一些工程师搞不清楚并发编程;还有工程师甚至常常写面条式的代码而不自知。如果每位工程师有自己的 checklist 来记录这些问题,定期回顾自己是不是还在犯类似的错误,他们的水平就会很快提高,至少不容易重复已经犯过的错误。同样,团队也需要积累 Code Review 的 checklist,刚开始,这个 list 可能非常初级,会有一些常见的 bad small,甚至代码规范的内容。不过没关系,相信随着团队成员能力的提升,这个 list 慢慢会集中在设计方面,比如编写代码的工程师有没有考虑到代码可维护性、扩展性和性能等等。
-
完善CI/CD设施。很多团队不愿意做 Code Review,其实和不愿意做单元测试、集成测试原因一样,这些团队的CI/CD 工具不成熟,每增加一个不直接产出“功能代码”的步骤就会增加工作负担。其实根本原因是工程效率工具的缺失。
-
培养工程师的能力。还有一个比较常见问题是,有些新人面对被 review 代码往往提不出问题。这个时候就需要工程师提升自己的能力。如果平时有积累,面对等待 review 的代码,即使不能面面俱到,也能提供不少有价值的意见,比如整理学习 Restful API 知识,在评审 Http 接口代码时就会是专家;掌握了 Spring 框架,Guava 等工具类,也能在很多时候提出很好的建议。慢慢地积累更多经验后,这些工程师就能提出更多业务、设计方面的意见。
2.3、Code Review 的实际操作建议
-
代码审查是应该在互相沟通中进行讨论的,而不是相互对抗。预先确定哪些是要点哪些不是,可以减少冲突并拟定预期。
-
经常进行Code Review, 不要攒了1w行才让同事帮你review, 这是坑队友.
- 要Review的代码越多,那么要重构,重写的代码就会越多。而越不被程序作者接受的建议也会越多,唾沫口水战也会越多。
- 程序员代码写得时候越长,程序员就会在代码中加入越来越多的个人的东西。 程序员最大的问题就是“自负”,无论什么时候,什么情况下,有太多的机会会让这种“自负”澎涨开来,并开始影响团队影响整个项目,以至于听不见别人的建议,从而让Code Review变成了口水战。
- 越接近软件发布的最终期限,代码也就不能改得太多。
-
Code Review不要太正式,而且要短
-
尽可能的让不同的人Reivew你的代码(不要超过3个人)
- 从不同的方向评审代码总是好的。
- 会有更多的人帮你在日后维护你的代码。
- 这也是一个增加团队凝聚力的方法。
-
保持积极的正面的态度
无论是代码作者,还是评审者,都需要一种积极向上的正面的态度,作者需要能够虚心接受别人的建议,因为别人的建议是为了让你做得更好;评审者也需要以一种积极的正面的态度向作者提意见,因为那是和你在一个战壕里的战友。记住,你不是一段代码,你是一个人!
-
学会享受Code Reivew
-
如何做一个令人尊敬的代码评审人员
当项目维护者不喜欢贡献者提交的代码,经常会出现这种情况。在最好的情况下,这种代码评审策略会导致无意义的争论。在最坏的情况下,它会阻碍项目贡献,形成一个充满敌意和精英主义的文化。
代码评审应该是客观和简洁的,并尽可能面向确定性。代码评审不是关于政治或情感上的争论,而是一个技术问题。代码评审的目标应该是不断进取,提升项目及其参与者的水平。提交的代码应该根据代码的优点而不是对提交者的意见来评判。
代码评审策略
以下是一些代码评审策略,作为项目维护者,如果你看到不喜欢的代码,可以尝试应用这些评审策略。
1. 把拒绝变成问问题
糟糕的评审:“如果你这样改,就不可能……”。(这显然有点夸张,真的不可能吗?)好的评审:“如果你这样改,那该怎么……?”
2. 避免夸大其词
简单一点,把你的顾虑和问题说出来,这样有助于你获得期望的结果。
糟糕的评审:“这个变更对性能影响很大。”好的评审:“这样改的话性能会比之前低一些,你有做过测试吗?”更好的评审:“我会准备一些数据来验证一下这样改之后速度不会比之前慢。”或者这样:“这个变更把之前的 O(n) 变成了 O(n2),不会影响性能吗?”
3. 把带有讽刺意味的评语留给你自己
有些想法最好把它烂在肚子里,如果你不想让人觉得你粗鲁,最好不要把这些想法说出来。
“我觉得这个变更太糟糕了,它会毁了所有东西的。”“你真的觉得软件工程这个行业适合你呆下去吗?”
4. 积极参与
对于同一个问题,或许你会有不同的想法。如果你能够积极参与,可能会得到比之前更好的解决方案。
糟糕的评审:“这个想法太糟糕了,我有更好的解决方案。”好的评审:“对于这个问题我也有一些类似的想法,或许我们可以比较或者组合一下我们的想法。”或者:“我也有一些类似的想法,我这样做是因为……而你那么做是因为什么?”
5. 不是每个人的经历都跟你一样
有些东西对你来说是常识,但有些人可能并不知道,即使他们的能力并不差。你可以说这些东西是常识,但不要显露出嘲笑或高人一等的姿态。
糟糕的评审:“你不知道这个明显是错的吗?”好的评审:“这样是不对的,因为当……的时候它会抛出空指针异常。”
6. 不要把复杂的东西简单化
有些事情对你来说是显而易见的,但对其他人并不一定也是这样。为别人提供可选方案或有用的例子可以让他们也变得跟你一样好。
糟糕的评审:“为什么不直接这样?”好的评审:“这样做会更简单,比如……”
7. 尊重别人
有时候,提交的代码可能质量很差,但表示一下对别人的尊重其实很容易。
糟糕的评审:“这些愚蠢的代码人跟写代码的人一样愚蠢。”好的评审:“感谢你提交的代码,但我们不能接受它们,因为有一些问题(已经列出来了)。”或者:“代码有一些问题,已经列出来了。或许我们可以回退一步,一起讨论一下用例?这样可以帮我们往前进一步。”
8. 管理你的期望和时间
如果一次提交的代码太多,你没有时间评审,可以让提交者知道。
糟糕的评审:“代码太多了,我不会合并它们的。”同样糟糕的是直接忽略它们。好的评审:“可以把这些分成几次提交吗?我没有这么多时间,而且一次也评审不了这么多代码。”
9. 使用礼貌用语
使用礼貌用语(比如“请”)表示对代码提交者的尊重,特别是当你要他们在细节上做出一些调整(比如格式化)时。
“请你把与空格相关的变更放在单独的 PR 里,可以吗?”“请你把变量对齐,这样更容易阅读,可以吗?”
10. 开启对话
你可能照着上面所说的去做了,但对提交的代码仍然不满意。这个时候你可以这么说:“我不喜欢这段代码,但我也不知道为什么,我们可以聊聊吗?”尽管需要多花一点时间,但这样是值得的,因为这样你和对方都能学到东西(一个讲一个听),而不是变成对立方。
即使是很有经验的程序员也可以这么说:“我也不知道为什么不喜欢这段代码”。这不是在创造攻击代码评审人员的机会,而是打开一条共同求知的道路。
总 结
避免使用双关语或夸大其词,避免争论,避免精英主义或贬低他人,避免诸如“显而易见”和“你为什么不……”这样的评语。使用清晰、真实的陈述和支持性的语言,提出问题,推动事情向前发展。记住,同事和代码贡献者都是人,他们的付出和你的付出一样值得尊重。
三、什么时候、如何进行代码审查
3.1、我的建议
1、个人:每一次代码提交时的强制三人左右的代码评审。
2、会审:以项目为单位,召开专门的代码评审会议。
3、团队:代码走查,团队成员互相检查代码,小团队一周一次,大团队一个月一次。
3.2、其他借鉴
https://www.cnblogs.com/DinoFung/articles/2302964.html
四、Code Review 时该关注的
4.1、体系结构和代码设计
-
代码复用: 根据“三振法”, 如果代码被复制一次,虽然不喜欢这种方式,但通常没什么问题。但如果再一次被复制,就应该通过提取公共的部分来重构它。
-
用更好的代码: 如果在一块混乱的代码做修改,添加几行代码也许更容易,但建议更进一步,用比原来更好的代码。
-
潜在的bugs: 是否会引起的其他错误?循环是否以我们期望的方式终止?
-
错误处理: 错误确定被优雅的修改?会导致其他错误?如果这样,修改是否有用?
-
效率: 如果代码中包含算法,这种算法是否是高效? 例如,在字典中使用迭代,遍历一个期望的值,这是一种低效的方式。
-
新代码与全局的架构是否保持一致?
-
基础代码是否有结合使用了一些标准或设计样式,新的代码是否遵循当前的规范?代码是否正确迁移,或参照了因不规范而淘汰的旧代码?
-
代码的位置是否正确?比如涉及订单的新代码是否在订单服务相关的位置?
-
新代码是否重用了现存的代码?新代码是否可以被现有代码重用?新代码是否有重复代码?如果是的话,是否应该重构成一个更可被重用的模式,还是当前还可以接受?
-
新代码是否被过度设计了?是否引入现在还不需要的重用设计?
4.2、可读性和可维护性
-
字段、变量、参数、方法、类的命名是否真实反映它们所代表的事物, 是否能够望文生义?
-
是否可以通过读代码理解它做了什么?
-
是否理解测试用例测了什么?
-
测试是否很好地覆盖了用例的各种情况?它们是否覆盖了正常和异常用例?是否有忽略的情况?
-
错误信息是否可被理解? log打的是否正确和足够?
-
不清晰的代码是否被文档、注释或容易理解的测试用例所覆盖?具体可以根据团队自身的喜好决定使用哪种方式。
4.3、功能
-
代码是否真的达到了预期的目标?如果有自动化测试来确保代码的正确性,测试的代码是否真的可以验证代码达到了协定的需求?
-
代码看上去是否包含不明显的bug,比如使用错误的变量进行检查,或误把and写成or?
-
作者是否需要创建公共文档或修改现存的帮助文档?
-
是否检查了面向用户的信息的正确性?
-
是否有会在生产环境中导致应用停止运行的明显错误?代码是否会错误地指向测试数据库,是否存在应在真实服务中移除的硬编码的stub代码?
-
你对性能的需求是什么,你是否考虑了安全问题?
- 这个新增或修复的功能是否会反向影响到现存的性能测试结果
- 外部调用很昂贵(a. 数据库调用. b. 不必要的远程调用. c. 移动或穿戴设备过频繁地调用后端程序)
4.4、安全
- 检查是否新的路径和服务需要认证
- 数据是否需要加密
-
密码是否被很好地控制?
这里的密码包含密码(如用户密码、数据库密码或其他系统的密码)、秘钥、令牌等等。这些永远不应该存放在会提交到源码控制系统的代码或配置文件中,有其他方式管理这些密码,例如通过密码服务器(secret server)。当审查代码时,要确保这些密码不会悄悄进入你的版本控制系统中
-
代码的运行是否应该被日志记录或监控?是否正确地使用?
日志和监控需求因各个项目而不同,一些需要合规,一些拥有比别人严格的行为、事件日志规范。如果你有规章规定哪些需要记录日志,何时、如何记录,那么作为代码审查者你应该检查提交的代码是否满足要求。如果你没有固定的规章,那么就考虑:
- 代码是否改变了数据(如增删改操作)?是否应该记录由谁何时改变了什么?
- 代码是否涉及关键性能的部分?是否应该在性能监控系统中记录开始时间和结束时间?
- 每条日志的日志等级是否恰当?一个好的经验法则是“ERROR”触发一个提示发送到某处,如果你不想这些消息在凌晨3点叫醒谁,那么就将之降级为“INFO”或“DEBUG”。当在循环中或一条数据可能产生多条输出的情况下,一般不需要将它们记录到生产日志文件中,它们更应该被放在“DEBUG”级别。
4.5、 其他方面
-
是否合理地释放了资源
- 是否存在内存泄漏?
- 是否存在内存无限增长? 例如, 如果审查者看到不断有变量被追加到list或map中, 那么就要考虑下这个list或map什么时候失效, 或清除无用数据
- 代码是否及时关闭了连接或数据流?
- 资源池配置是否是否正确? 有没有过大或者过小?
-
异常情况是否能够正确处理?
- 超时是否能够正确处理?
- 调用接口出错的时候, 是否有出错处理逻辑, 并且处理正确?
- 进程意外重启后, 是否能够恢复到崩溃前的环境?
-
正确性(主要与多线程环境关系密切)
- 代码是否使用了正确的适合多线程的数据结构
- 代码是否存在竞态条件(race conditions)?多线程环境中代码非常容易造成不明显的竞态条件。作为审查者,可以查看不是原子操作的get和set
- 代码是否正确使用锁?和竞态条件相关,作为审查者你应该检查被审代码是否允许多个线程修改变量导致程序崩溃。代码可能需要同步、锁、原子变量来对代码块进行控制
- 代码的性能测试是否有价值?很容易将小型的性能测试代码写得很糟糕,或者使用不能代表生产环境数据的测试数据,这样只会得到错误的结果
- 缓存:虽然缓存是一种能防止过多高消耗请求的方式,但其本身也存在一些挑战。如果审查的代码使用了缓存,你应该关注一些常见的问题,如,不正确的缓存失效方式
-
代码级优化, 对大部分并不是要构建低延时应用的机构来说,代码级优化往往是过早优化,所以首先要知道代码级优化是否必要
- 代码是否在不需要的地方使用同步或锁操作?如果代码始终运行在单线程中,锁往往是不必要的
- 代码是否可以使用原子变量替代锁或同步操作?
- 代码是否使用了不必要的线程安全的数据结构?比如是否可以使用ArrayList替代Vector?
- 代码是否在通用的操作中使用了低性能的数据结构?如在经常需要查找某个特定元素的地方使用链表
- 代码是否可以使用懒加载并从中获得性能提升?
- 条件判断语句或其他逻辑是否可以将最高效的求值语句放在前面来使其他语句短路?
- 代码是否存在许多字符串格式化?是否有方法可以使之更高效?
- 日志语句是否使用了字符串格式化?是否先使用条件判断语句校验了日志等级,或使用延迟求值?
五、参考知识
1、路径
https://blog.csdn.net/u013161431/article/details/82626651
https://coolshell.cn/articles/1302.html
https://www.zhihu.com/question/20046020
https://blog.csdn.net/ricohzhanglong/article/details/14649067
-
-
谷歌开源内部代码评审规范
2019-10-08 15:33:58近日,谷歌开源了其内部一直在使用的代码评审规范,InfoQ 对其进行了翻译和整理,分享给广大开发者,看看谷歌工程师是如何评审代码的。 代码评审标准 代码评审的主要目的是确保代码库的整体质量随时间...谷歌成立于 1998 年,以搜索起家,到目前为止已经发展了 21 年。在过去的 21 年中,谷歌不断创新,开发了七款产品,拥有超过 10 亿级活跃用户,谷歌的工程师文化一直被认为是优秀且特别的。近日,谷歌开源了其内部一直在使用的代码评审规范,InfoQ 对其进行了翻译和整理,分享给广大开发者,看看谷歌工程师是如何评审代码的。
代码评审标准
代码评审的主要目的是确保代码库的整体质量随时间推移逐步得到提升,所有代码评审工具和过程都是为了实现这一目标而设计的。
为了实现这个目标,必须做出一系列权衡。首先,开发人员的开发任务必须要有所进展。如果他们不提交改进的代码,代码库质量就得不到改善。此外,如果评审人员过于严格,开发人员就没有动力进行持续改进。
评审人员的职责是确保每个 CL(变更列表)的质量,保证代码库整体质量不会随着时间的推移而下降。这是一项艰巨的任务,因为代码库整体质量常常会随着每次提交代码质量的小幅下降而退化,特别是有时候开发团队时间很紧,并认为必须走捷径才能完成交付任务。
评审人员要对他们评审的代码负起责任,确保代码库保持一致性和可维护性。
以下是可在代码评审中使用的准则:
一般来说,如果 CL 达到可以提升系统整体代码质量的程度,就可以让它们通过了,即使它们可能还不完美。
这是所有代码评审准则的最高原则。
当然,也有例外的时候。例如,如果 CL 中包含了系统不需要的功能,那么即使代码写得很好,评审人员也可以拒绝让它们通过。
这个世界上没有“完美”的代码,只有更好的代码。评审人员不应该要求开发人员对 CL 中的每一个微小部分都进行细致入微的打磨,而应该在满足需求和变更重要性之间做出权衡。评审人员不应该追求完美,而应该追求持续改进。如果一个 CL 能够从整体上提高系统的可维护性、可读性和可理解性,那它就不应该仅仅因为它不够“完美”而被延迟几天甚至几周。
评审人员应该提供建议,告诉开发人员哪些方面可以做得更好。但如果这些建议不是很重要,可以在前面加上像“Nit:”这样的前缀,让开发人员知道这只是一个改进建议,他们也可以选择忽略。
指导
代码评审的一个作用是向开发人员传授知识,比如关于一门语言、一个框架或一般软件设计原则的知识。分享知识是提升系统代码质量的一个组成部分。但要注意,如果你的建议纯粹是带有教育性质的,并且对于满足本文所描述的标准来说并不是那么重要,那么请在前面加上“Nit:”,或者以其他方式告诉开发人员,他们并不一定要在 CL 中解决这些问题。
原则
-
客观的技术和数据比个人意见和偏好更重要。
-
在代码风格方面,可以参考谷歌风格指南( http://google.github.io/styleguide )。任何没有在这个风格指南中出现的东西(比如空格等)都属于个人偏好。代码风格应该与原有代码保持一致,如果之前没有规定代码风格,可以使用代码提交者的代码风格。
-
软件设计从来就不只风格问题,也不只是个人偏好问题。它们建立在一些基本原则之上,所以我们应该基于这些原则做出权衡,而不只是基于个人偏好。有时候,一个问题有多种解决方案,如果开发人员能够证明(通过数据或基于可靠的工程原理)几种解决方案是同样有效的,那么评审人员应该接受开发人员的选择,否则就应该基于软件设计标准原则做出决定。
-
如果没有其他适用的原则,评审人员可以要求开发人员与当前代码库保持一致,只要不破坏系统的整体代码质量。
解决冲突
在代码评审过程中出现冲突时,开发人员和评审人员首先要尝试根据本文、CL 作者指南( https://google.github.io/eng-practices/review/developer/ )和评审人员指南( https://google.github.io/eng-practices/review/reviewer/ )达成一致意见。
如果很难达成一致意见,评审人员和开发人员可以进行面对面会议或者视频会议,而不是只是试图通过代码评审评论板来解决冲突。
如果还不能解决问题,那么就要考虑把问题升级,进行更广泛的团队讨论。让团队负责人参与进来,请求代码维护人员作出决定,或请求工程经理提供帮助。不要因为开发人员和评审人员无法达成一致意见就让 CL 一直挂在那里。
代码评审要注意哪些事情?
设计
代码评审中最重要的部分是 CL 的总体设计。CL 中不同代码段之间的交互是有意义的吗?这个变更应该属于代码库,还是属于某个包?它与系统的其他部分可以良好地集成吗?现在是引入这个变更的好时机吗?
功能
这个 CL 是否达到了开发人员的目的?开发人员的意图对代码用户来说有好处吗?代码“用户”可以是指最终用户(他们受代码变更的影响)和开发人员(将来要“使用”这些代码)。
大多数情况下,我们希望开发人员先测试好 CL,确保它们能够正确运行。但作为评审人员,你仍然要考虑一些边缘情况,比如查找并发问题,尝试像用户一样思考问题,并找出只是通过阅读代码无法看到的错误。
如果愿意,你也可以验证一下 CL。如果一个 CL 会影响用户,比如做出了 UI 变更,那么这是验证 CL 的好时机。如果只是看代码,很难理解一些变更将如何影响用户。对于这样的更改,如果不方便自己运行,可以让开发人员提供功能演示。
另一个重要的考虑点是 CL 中是否存在可能导致死锁或竞态条件的并发问题。只是简单地运行代码很难发现这类问题,通常需要有人(开发人员和评审人员)仔细思考这些问题,确保不会把它们引入到系统中。
复杂性
CL 比实际需要的更复杂吗?从每一层面检查 CL,细到每一行代码,它们是不是太复杂了?函数是否过于复杂?类复杂吗?“太复杂”通常意味着“阅读代码的人难以很快理解它们”,也意味着“开发人员在调用或修改这些代码时可能会引入 bug”。
过度设计是一种特殊的复杂性,开发人员把代码写得比实际需要的更通用,或者增加了系统当前不需要的功能。评审人员要警惕过度设计,鼓励开发人员只解决现在需要解决的问题,而不是将来可能需要解决的问题。未来的问题应该在它们出现之后再去解决,因为到了那个时候我们可以看到它们的实际状况和需求。
测试
要求开发人员进行单元测试、集成测试或端到端测试。一般来说,CL 中应该包含测试,除非这个 CL 只是为了处理紧急情况。
确保 CL 中的测试是正确、合理和有用的。因为测试本身无法测试自己,而且我们很少会为测试编写测试,所以必须确保测试是有效的。
如果代码出了问题,测试会失败吗?如果代码发生改动,它们会误报吗?每一个测试都有断言吗?是否按照不同的测试方法对测试进行分类?
请记住,测试代码也是需要维护的。
命名
开发人员是否使用了良好的命名方式?好的命名要能够充分表达一个项(变量、类名等)是什么或者用来做什么,但又不至于让人难以阅读。
注释
开发人员有没有用自然语言写出清晰的注释?他们所写的注释都是必需的吗?通常,注释应该用于解释代码的用处,而不是解释它们在干什么。如果代码不够清晰,无法自解释,那就应该简化代码。当然也有一些例外(例如,正则表达式和复杂的算法,如果能够解释它们在做什么,会让阅读代码的人受益匪浅),但大多数注释都应该指出代码中不可能包含的信息,比如这些代码背后的缘由。
CL 附带的其他注解也很重要,比如告知一个可以移除的待办事项,或者一个不要做出代码变更的建议,等等。
注意,注释不同于类、模块或函数文档。文档的目的是为了说明代码的用途、用法和行为。
代码风格
谷歌为主要编程语言和大多数次要编程语言提供了代码风格指南( http://google.github.io/styleguide/ ),所以要确保 CL 遵循了适当的指南。
如果你想对指南中没有提及的风格做出改进,可以在注释前面加上“Nit:”,让开发人员知道这是一个你认为可以改进的地方,但不是强制性的。但请不要只是基于个人偏好来阻止 CL 的提交。
开发人员不应该将风格变更与其他变更放在一起,这样很难看出 CL 发生了哪些变化,导致合并和回滚变得更加复杂。如果开发人员想要重新格式化整个文件,让他们将重新格式化后的文件作为单独的 CL,并将功能变更作为另一个 CL。
文档
如果 CL 导致用户构建、测试、交互或发布代码的方式发生了变化,请确保相关的文档也得到了更新,包括 README、g3doc 页和其他生成的参考文档。如果 CL 有移除或弃用代码,请考虑一下是否也应该删除相关的文档。如果文档缺失,要向开发人员索要。
查看每一行代码
查看每一行代码。有些东西可以看一看,比如数据文件、生成的代码或大型数据结构,但不要只是粗略地扫一下类、函数或代码块,并假定它们都能正常运行。显然,有些代码需要仔细检查,至于是哪些代码完全取决于你,但你至少应该要理解这些代码都在做些什么。
如果代码很复杂或者你难以快速看懂它们,导致评审速度变慢,你要让开发人员知道,并在进行进一步评审之前让他们做一些澄清。如果你看不懂这些代码,其他开发人员很可能也看不懂。因此,要求开发人员澄清代码其实也是在帮助未来的开发人员更好地理解代码。
如果你理解代码,但又觉得没有资格做代码评审,可以确保有资格的 CL 评审人员在代码评审时考虑到了安全性、并发性、可访问性、国际化等问题。
上下文
代码评审工具通常只显示被修改的代码,但有时候你需要查看整个文件,确保代码变更是有意义的。例如,你可能只看到新添加了四行代码,但如果你看一下整个文件,会发现这四行代码位于一个 50 多行的方法中,这个时候需要将这个方法拆分为更小的方法。
你需要基于整个系统来考量 CL。这个 CL 是提升了系统的代码质量,还是让整个系统变得更复杂、更不可测?不要接受导致系统代码质量退化的 CL。大多数系统都是因为累积了很多小的变更而变复杂的,所以要尽量避免小的变更带来的复杂性。
好的一面
如果你在 CL 中看到一些不错的东西,要让开发人员知道,特别是当他们以一种很好的方式解决了问题。代码评审通常只关注错误的东西,但其实也应该鼓励和赞赏好的代码实践。有时候,让开发人员知道他们做对了事情比让他们知道做错了事情更有价值。
总结
在进行代码评审时,你要确保:
- 良好的代码设计。
- 功能对代码用户来说是有用的。
- UI 变更应该是合理的。
- 并行编程是安全的。
- 代码复杂性不要超过应有的程度。
- 不需要实现可能会在未来出现的需求。
- 有适当的单元测试。
- 精心设计的测试用例。
- 使用了清晰的命名方式。
- 清晰而有用的代码注释,要解释“为什么”,而不是“什么”。
- 恰如其分的代码文档化。
- 代码要遵循风格指南。
检查每一行代码,查看上下文,确保你正在改进代码质量,并为表现不错的开发人员点赞。
检查 CL
在知道了代码评审要关注哪些东西之后,如何有效地进行跨文件代码评审呢?
- 代码变更有意义吗?它们有没有良好的描述?
- 先看一下代码变更中最重要的部分,它整体设计得如何?
- 按照适当的顺序检查 CL 的其余部分。
第一步:从整体查看代码变更
先看一下 CL 描述,看看这个 CL 做了些什么。做出这个变更有意义吗?如果这个变更是不必要的,请立即做出回复,并解释为什么不应该发生这个变更。在你拒绝这样的变更时,可以向开发人员建议他们应该做些什么。
例如,你可以说:“看起来你在这方面做得不错,谢谢!不过,我们正打算移除这个系统,所以现在不想对它做任何修改。或许你可以重构一下另外一个类”?
注意,评审人员在拒绝一个 CL 并提供替代建议时要做得很有礼貌。礼貌是很重要的,因为作为开发人员,我们要彼此尊重,即使可能意见不一致。
如果有很多 CL 是你不希望出现的,就要考虑重新调整开发团队或外部贡献者的开发流程,以便在开发新的 CL 之前进行更多的沟通。提前告诉人们哪些事情不要做,这比等他们做完了这些事情再把它们扔掉或者进行彻底重写要好得多。
第二步:检查 CL 的主要部分
找到 CL 的主要文件。通常一个 CL 会有一个包含了主要逻辑变更的文件,也就是 CL 的主要部分。先看看这些主要部分,有助于了解整个上下文,加快代码评审速度。如果 CL 太大,以致于你无法确定哪些部分是主要的,可以询问开发人员,或者让他们把 CL 拆分成多个 CL。
如果 CL 的主要部分存在严重的设计问题,要立即回复开发人员,即使你还没有时间检查 CL 的其余部分。这个时候检查 CL 的其余部分可能是在浪费时间,因为如果主要部分存在严重的设计问题,那么其他部分就变得无关紧要了。
为什么要立即回复开发人员?原因有二:
-
开发人员在发出一个 CL 之后会继续开始后续的开发工作。如果你正在评审的 CL 存在严重的设计问题,他们也需要重写后续的 CL。所以,最好赶在开发人员在有问题的设计上花费不必要的时间之前告诉他们。
-
大的设计变更比小的变更需要更长的时间。为了让开发人员能够在截止日期之前提交代码,同时又能保持代码库的质量,要尽早让他们开始重写工作。
第三步:按照适当的顺序检查 CL 的其余部分
在确认整体 CL 没有严重的设计问题之后,试着按照某种逻辑顺序来检查其他文件,确保不会错过任何一个需要检查的文件。通常,在你检查完主要文件之后,按照代码评审工具显示它们的顺序来浏览每个文件就可以了。你也可以在检查主要代码之前先查看测试代码,这样可以对代码变更有一个大致的概念。
代码评审的速度
为什么代码评审要快速进行?
在谷歌,我们对开发团队的整体交付速度(而不是针对个体开发人员写代码的速度)进行了优化。个体开发速度也很重要,但其重要性比不上整个团队的开发速度。
如果代码评审的速度很慢,就会发生以下这些事情:
-
团队的整体开发速度降低了。如果个体开发人员无法快速地对评审做出响应,可能是因为他们有其他事情要做。但是,如果每个 CL 都要等待一次又一次的评审,那么其他成员的新特性和 bug 修复就会被延迟,可能是几天、几周甚至是几个月。
-
开发人员开始对代码评审流程提出抗议。如果评审人员要隔几天才回复一次,但每次都要求对 CL 进行重大修改,开发人员可能会觉得很沮丧。通常,他们会抱怨评审人员太过严苛。如果评审人员能够快速提供反馈,抱怨就会消失,即使他们要求做出的修改是一样的。代码评审过程的大多数抱怨实际上可以通过加快评审速度来解决。
-
代码质量受影响。如果评审速度很慢,开发人员的压力也会随之增加,因为他们不能提交不甚完美的 CL。缓慢的评审流程还会阻碍代码清理、重构和对现有 CL 做出进一步改进。
代码评审应该要多快?
如果你不是在集中精力完成手头的任务,那就应该在第一时间评审代码。
对代码评审做出响应最好不要超过一个工作日。
如果遵循这些原则,那么一个典型的 CL 在一天内(如果需要的话)可以进行多轮评审。
速度和中断
有一种情况,即如果你正在集中精力完成手头的任务,比如写代码,那就不要打断自己去做代码评审。研究表明,开发人员被中断之后可能需要很长时间才能恢复到之前的状态。因此,从团队整体上看,在写代码时打断自己比让另一个开发人员等待代码评审要付出更大的代价。
所以,对于这种情况,可以等到你手头工作可以停了再开始代码评审。可以是在完成手头的编码任务之后,午饭后,会议结束后,休息结束后等。
快速响应
我们所说代码评审速度指的是响应时间,而不是 CL 完成整个评审过程并提交到代码库所需的时间。理想情况下,整个评审过程也应该是很快的,但单次评审请求的响应速度比整个过程的响应速度更重要。
有时候可能需要很长时间才能完成整个评审过程,但在整个过程中评审人员的快速响应可以极大减轻开发人员对“慢”评审的沮丧感。
如果你太忙了,可以先向开发人员发送一个响应,让他们知道你什么时候可以开始评审,或者建议让其他可以更快做出响应的评审人员来评审代码,或者提供一些初步反馈。
最重要的是评审人员要花足够的时间进行评审,确保代码符合标准。但不管怎样,最好响应速度还是要快一些。
跨时区代码评审
在进行跨时区代码评审时,试着在开发人员还在办公室的时候做出响应。如果他们已经回家了,那么最好可以确保他们在第二天回到办公室时可以看到代码评审已经完成。
带有注解的 LGTM
为了加快代码评审速度,对于以下两种情况,评审人员应该给出 LGTM(Look Good to Me,没有问题)或者通过,即使他们在 CL 中留下了未解决的问题:
- 评审人员确信开发人员将会处理好评审人员给出的建议和意见。
- 其余的改动是次要的,不一定要求开发人员完成。
当开发人员和评审人员处于不同的时区时,最好可以使用带有注解的 LGTM,否则开发人员可能需要等上一整天才能获得“LGTM,批准”。
大型的 CL
如果有人给你发了一个很大的代码评审,而你不确定是否有足够时间完成评审,通常的做法是要求开发人员把 CL 拆分成几个较小的 CL。这样做通常是合理的,对评审人员来说是有好处的,即使开发人员需要做点额外的工作。
如果一个 CL 不能拆分成更小的 CL,并且你没有足够的时间进行快速评审,至少要对 CL 的总体设计写一些注解,并发给开发人员。评审人员的目标之一是在不影响代码质量的情况下快速对开发人员做出响应,或者让他们能够快速采取进一步行动。
持续改进代码评审
如果你遵循了这些指导原则,并且对代码评审过程严格要求,你会发现,随着时间的推移,整个代码评审过程会变得越来越快。开发人员知道为了保证代码质量需要做些什么,并从一开始就向你发送非常棒的 CL,这样评审所需的时间就会越来越少。评审人员也学会了如何快速做出响应。但不要为了提高评审速度而牺牲代码评审标准或质量——从长远来看,这样做并不会让任何事情变得更快。
紧急情况
在一些紧急情况下,CL 必须非常快速地通过整个评审过程,在质量方面会有些许的放松。请参看这些“紧急情况”,看看哪些符合紧急情况标准,哪些不符合。
评审注解
概要
- 礼貌。
- 解释你的理由。
- 给出明确的方向,指出问题,并让开发人员决定如何在两者之间做出权衡。
- 鼓励开发人员简化代码,或者添加代码注释,而不只是让他们解释代码的复杂性。
礼貌
一般来说,礼貌和尊重是很重要的。一个是要确保你的评论是针对代码而不是针对开发人员。你不一定要一直这么做,但当你想说一些可能会让开发人员感到激动或有争议的话时,绝对有必要这么做。例如:
不好的说法:“为什么你要在这个地方使用线程,这样做显然不会获得任何好处”。
好的说法:“在这里使用并发模型增加了系统复杂性,但我看不到任何实际的性能好处,所以这段代码最好使用单线程,而不是多线程”。
解释理由
从上面的正面示例可以看出,这样有助于开发人员理解你为什么要给出这些建议。你并不一定总是要在评审中提供这些信息,但如果你能够为你的意图、所遵循的最佳实践或你的建议将如何改进代码质量给出更多的解释会更好。
给予指导
一般来说,修复 CL 是开发人员的责任,而不是评审人员的责任。你不需要为开发人员提供详细的解决方案或者为他们写代码。
不过,这并不意味着评审人员就不应该帮助开发人员。你最好可以在指出问题和给予指导之间做出权衡。指出问题,并让开发人员做出决策,这样有助于开发人员学到东西,并让代码评审变得更容易。这样还可以产出更好的解决方案,因为开发人员比评审人员更了解代码。
不过,有时候直接给出指令、建议或代码会更有用。代码评审的主要目的是获得尽可能好的 CL。第二个目的是提高开发人员的技能,这样以后需要的评审就会越来越少。
接受注解
如果你要求开发人员解释一段你不理解的代码,他们通常会去重写代码,并把代码写得更清晰。有时候在代码中添加注解也是一种恰当的做法,只要它不只是用来解释太过复杂的代码。
不要只是把注解写在代码评审工具里,因为这对于将来要阅读代码的人来说并没有多大帮助。只有少数情况可以接受这种做法,例如,你对评审的东西不太熟悉,而开发人员的解释却是很多人所熟知的。
代码评审回推
有时候,开发人员会回推代码评审。他们可能不同意你的意见,或者他们抱怨你太严格了。
谁是对的?
如果开发人员不同意你的意见,先花点时间想一下他们是不是对的。通常,他们比你更熟悉代码,所以可能对代码的某些方面更了解。他们的论点有道理吗?从代码质量角度来看,他们的回推是有道理的吗?如果是,就让他们知道他们是对的,这个问题就解决了。
然而,开发人员并不总是正确的。在这种情况下,评审人员要进一步解释为什么他们的建议是正确的。
如果评审人员认为他们的建议可以改善代码质量,并认为评审所带来的代码质量改进值得开发人员做出额外的工作,那么他们就应该坚持。改善代码质量往往是由一系列的小步骤组成的。
有时候你需要花很多时间反复解释,但要始终保持礼貌,并让开发人员知道你知道他们在说什么。
激动的开发人员
有时候,评审人员会认为如果他们坚持要开发人员做出改动,会让开发人员感到不安。开发人员有时候确实会感到沮丧,但这种感觉通常都很短暂,之后他们会非常感谢你帮助他们提高了代码质量。如果你在评审过程中表现得很有礼貌,开发人员一点都不会感到不安,这种担心可能是多余的。通常,令开发人员感到不安的是你写注解的方式,而不是你对代码质量的坚持。
稍后再解决
一种常见的回推原因是开发人员希望尽快完成任务。他们不想经过一轮又一轮的代码评审,他们说他们会在后续的 CL 中解决遗留问题,你现在让 CL 通过就可以了。一些开发人员会做得很好,他们在提交 CL 后立即就开发后续的 CL。但经验表明,开发人员开发原始 CL 的时间越长,他们进行后续修复的可能性就越小。除非开发人员在提交 CL 之后立即进行修复,否则在通过之后通常不会再去做这件事情。这并不是因为开发人员不负责任,而是因为他们有很多工作要做,而修复工作通常会被遗忘。所以,最好让开发人员马上把 CL 修复掉。
如果 CL 引入了新的复杂性,在提交之前必须将其清理掉,除非是紧急情况。如果 CL 暴露了一些目前还无法解决的问题,开发人员需要把 bug 记录下来,并将其分配给自己,这样它就不会被遗漏。他们还可以在代码中加入 TODO 注释,指向已经记录好的 bug。
抱怨评审太严格
如果你之前的代码评审很放松,然后突然变得严格起来,可能会引起一些开发人员的抱怨。不过没关系,加快代码评审速度通常会让这些抱怨逐渐消失。
有时候,这些抱怨可能需要几个月的时间才能消除,但开发人员到最后通常会看到代码评审的价值,因为他们看到了严格的代码评审有助于产出优秀的代码。有时候,抗议最大声的人甚至会成为你最坚定的支持者。
解决冲突
如果你遵循了上述方法,但仍然会在评审过程中遇到无法解决的冲突,请再次参阅代码评审标准,了解那些有助于解决冲突的指导原则。
原文链接: https://google.github.io/eng-practices/review/reviewer/
-
-
代码评审|阿里巴巴DevOps实践指南
2022-01-19 15:18:18代码评审,英文名是 Code Review,简称 CR,它是结对编程相互切磋相互学习的方式。严肃地讲,CR能够提升代码质量、促进人才成长、培养技术情怀。 -
说透代码评审
2020-07-20 23:01:01Java面试笔试面经、Java技术每天学习一点Java面试关注不迷路作者:张飞洪来源:https://www.cnblogs.com/jackyfei代码如熵,不加外力很容易就会随着代码...