开源,不只是代码,可以是工程实践:Google代码评审指南及其深刻的启发
最近了解到刚成立不久的、国内第一支开源基金——开放原子开源基金会,就提倡不只是开源软件,而且开源硬件、开源芯片、开源内容等,今天就来谈谈软件工程实践的开源。
开源软件工程实践,首先是可以通过开源代码——即开源一个工具来实现,有些工具就是固化的工程实践,如阿里开源的工具ChaosBlade,实际就是把阿里的混沌工程实践开源出去了。
来源:https://github.com/chaosblade-io/chaosblade
像内容开源,最有代表的就是维基百科、百度百科等,专业的内容开放还包括一些单项技术的文档网站,如https://react.docschina.org/ 包含了文档、教程和社区等。
之前本公众号也介绍过完整的谷歌软件工程之道、谷歌质量管理之道等,今天在这里详细介绍谷歌(Google)开源的代码评审(Code Review)最佳实践,即:Google's Code Review Guidelines(https://github.com/google/eng-practices),包含了两部分:
The Code Reviewer's Guide(代码评审人员指导)
The Change Author's Guide(变更作者指导)
The Standard of Code Review(代码评审标准)
What to Look For In a Code Review(代码评审的目标)
Navigating a CL(changelist) in Review(代码评审中浏览变更列表CL)
Speed of Code Reviews(代码评审的速度)
How to Write Code Review Comments(如何写审查的评论)
Handling Pushback in Code Reviews(如何处理驳回)
代码评审最主要的目的是确保代码库一直保持「健康」的状态、或让所有代码成为整洁的代码,希望检查出开发初期未察觉的一些错误,从而提升代码质量,包括代码一致性、性能优化、缩短代码运行时间,这很有挑战,因为代码规模总是在不断增大。所有的代码都要被review一遍,否则无法check in到代码库中。如果评审人很难提出问题,那么开发人员就没有动力提交、改进。进入良性循环,需要双方努力。
代码评审的标准有以下几条:
没有“完美”的代码,只有更好的代码。评审人应该平衡取得进步的需求,而不应要求作者做到完美才能被批准。评审人不寻求完美,而是持续改进。 即使CL(变更列表)并不完美,一旦肯定可以改善现有系统的整体代码运行状况,评审人就应该批准它。 如果CL添加了评审人不希望在其系统中使用的功能,则即使代码设计得当,评审人也可以肯定地拒绝批准。 评审人应该随时发表评论,表示可能会更好。 以技术事实和数据为依据,而不是个人的观点和个人喜好。 代码风格指南是绝对权威,如果之前某种风格没有要求,请接受作者的风格。 如果作者可以证明(通过数据或基于可靠的工程原理)几种方法同等有效,那么评审人应接受作者的偏爱
设计:代码是不是经过精心的设计,并适合现有的系统?
功能性:代码的行为是否和作者的意图保持一致?代码的行为方式对用户是否正常?
复杂度:代码能更简单一些吗?在未来,其它开发者能更容易地理解并使用这些代码吗?
测试:代码是否正确?是否通过了精心设计的自动测试?
命名:开发者是否选择易于理解的名称给变量、类和方法进行命名?
注释:代码注释是否足够清晰并有用?
风格:代码是否采用了标准的编程风格?
文档:开发者是否更新了相关的文档?
Be kind (友好).
{#why} Explain your reasoning (解释为什么).
{#guidance} Balance giving explicit directions with just pointing out problems and letting the developer decide(一方面针对所指出的问题给出明确方向,另方面由开发人员自己做决定,两者之间达到平衡).
Encourage developers to simplify code or add code comments instead of just explaining the complexity to you(鼓励开发人员简化代码或为代码加注释,而不是解释代码的复杂性).
Writing Good CL Descriptions(写好变更列表描述)
Small CLs(构建一些小的变更列表)
How to Handle Reviewer Comments(如何处理评审人建议)
CL的描述至关重要,它是对所做的更改以及更改原因的公开记录,将成为版本控制历史的永久组成部分,除了评审人需要通过阅读它而理解代码的修改,未来还有数百人可能会搜索它、阅读它,对读者的清晰度和实用性应该是头等大事。对CL描述的第一行 {#firstline} 的要求:
简要说明所做的修改。
完整的句子,写得好像是命令(命令式句子,例如"Delete the FizzBuzz RPC and replace it with the new system." )。
后面要留一空行。
第一行应该独立存在,而且要简短、专心、切题,从而使读者可以更快地浏览代码历史记录。而说明部分 {#informative}是提供信息的,它可能包括对要解决的问题的简要说明,以及为什么这是最好的方法,包括相关的背景信息,如Bug编号、性能测试结果、设计文档的链接等。
针对这个,还举了一些不好的例子,例如"Fix bug" 就很不充分,改了哪一个Bug?如何修改这个Bug的 或为了改这个Bug,做了什么?类似糟糕的例子还有:
"Fix build."
"Add patch."
"Moving code from A to B."
"Phase 1."
"Add convenience functions."
"kill weird URLs."
rpc:删除RPC服务器消息空闲列表的大小限制。
像FizzBuzz这样的服务器具有非常大的消息,可以从重用中受益。增大自由列表,并添加一个goroutine,以便随着时间的推移缓慢释放自由列表条目,以便空闲服务器最终释放所有自由列表条目。
使用TimeKeeper构造一个任务以使用其TimeStr和Now方法。
向Task中添加一个Now方法,以便可以删除borglet()取值方法(仅OOMCandidate用来调用borglet的Now方法)。这将替换Borglet上委托给TimeKeeper的方法。
即使对一些小的变更列表,也要给出上下文。而为什么要编写小型CL{#why}? 给出了很多理由,如审核更快、更彻底的评审、引入错误的可能性较小、减少浪费、易于合并、易于设计、加快进程、回滚更简单等。
什么是小的变更 {#what_is_small}?一般认为只解决一个问题,100-200行代码比较合适,而1000行代码就太大。如果更改比较大,会按文件分割{#splitting-files} 或 单独进行重构{#refactoring},包括做一些清理工作,如修复本地变量名称。
Google重视单元测试,强调将相关的测试代码保存在相同的CL{#test_code}中,使用新测试验证先前存在的提交代码,并确保测试涵盖了重要的逻辑,这样提高了对受影响代码进行后续重构的信心。需要时,还要重构测试代码(例如,引入辅助函数)、引入更大的测试框架代码(例如集成测试)等。
评审的目的是保持代码库和产品的质量。当评审人对开发人员的代码提出批评时,开发人员要将其视为帮助自己,而不是对个人 {#personal} 能力的人身攻击。即使评审人在评论中表达了沮丧(虽然这不是一个好习惯),但是作为开发人员应该事先有心理准备而问自己:“评审人试图与我交流的建设性内容是什么?” 然后像他们实际所说的那样操作,切勿激怒而表现在回复的注释中,这严重违反了专业礼节,而且将永远存在于代码审查工具中。如果开发人员太生气或烦恼而无法友好地回应,则先离开计算机一段时间或进行其他操作,直到感到足够镇静以至于礼貌地回答。类似这样详细的指导,还不止这一个地方。
如果评审人不理解开发人员的某些代码,也意味着将来还有很多人也可能不理解。所以第一反应应该是澄清代码本身或添加代码注释,达到修复代码{#code}的目的。当开发提交代码时,多数情况下是很自信的,容易认为评审人的评论是错误的,但这不好,开发人员应该退一步、花点时间思考{#think},第一个问题总是问自己:“审稿人是否正确?” ,反思:评审人是否提供了有价值的反馈、对代码库和Google是否有所帮助?
启发:
大家看到这里,是不是感觉Google对代码评审不仅重视、严格,而且指导有方、细致?另外,还发现什么了吗?“细致、tag(如 {#...}等” 非常有助于确保代码提交、评审的各种注释/评论的文字质量,这有什么好处?有,在Google使用一个代码库,当数据积累比较多的时候,就可以用人工智能(AI)的技术来帮助推荐代码、修改缺陷。现在许多公司应用AI技术,其效果差的原因就是数据质量的问题,而数据质量直接由这些代码注释、评审意见决定。实际上,Google的评审工具中已经在使用AI技术,如工具通过查看发生变更的代码的所有权和作者、近期评审人员的历史、每个潜在的评论人员的待审查的代码数量、代码注释、评审意见等来自动推荐出合适的评审人员、推荐合适的代码和解决方案。