精华内容
下载资源
问答
  • 前几天看了《CodeReview程序员的寄望与哀伤》,想到我们团队开展CodeReview也有2年了,结果还算比较满意,有些经验应该可以和大家一起分享、探讨。我们为什么要推行CodeReview呢?我们当时面临着代码混乱、Bug频出的...
  • Google是如何Code Review

    千次阅读 多人点赞 2019-10-24 10:18:40
    最好在大量工作之前告诉人们“不必”,因为现在要将其丢弃或彻底重写。 第二步: 检查CL的主要部分 查找属于此CL的“主要”部分的文件。通常来说一个CL最重要的部分是它逻辑修改数最多的那个文件。这样有助于...

    我和几个小伙伴一起翻译了Google前一段时间放出来的Google’s Engineering Practices documentation,翻译后的github仓库https://github.com/xindoo/eng-practices-cn,欢迎加star。目前只是翻译完了,因为译者水平有限,还需要审校。另外后续google肯定还会有新内容放出来,欢迎想参与贡献的小伙伴加入,也欢迎在github上加star。
    这篇文章是Google’s Engineering Practices documentation的第一章Code Review实践指南

    术语

    部分文档中会用到一些谷歌内部的术语,特在此说明:

    • CL: “changelist"的缩写,代表已经进入版本控制软件或者正在进行代码评审的变更。
      其他组织经常称为"change"或者"patch”。
    • LGTM: "Looks Good to Me."的缩写,看起来不错,评审者批准CL时会这么说。

    评审者指南

    Code Review标准

    Code Review的主要目的是始终保证随着时间的推移,谷歌代码越来越健康,所有Code Review的工具和流程也是针对于此设计的。

    为了完成这点,我们不得不权衡利弊。

    首先,开发者应当在他们代码中做一些 改进 ,如果你永远都不做出改进,代码库整体质量也不会提升。但是如果审查者为难所有变更,开发者未来也会失去改进的动力。

    另一方面,保证代码库随时间推移越来越健康是审查者的责任,而不是让代码库质量变得越来越差。这很棘手,因为代码质量一般都会随着时间推移越来越差,尤其是在团队有明确时间限制、而且他们觉得不得不采取一些投机取巧的方式才能完成任务的情况下。

    但是,代码评审者得对他们Review的代码负责,所以他们想始终确保代码库一致、可维护(其他指标见"Code Review应该关注什么?"

    依据这些,我们将以下准则作为我们期望的Code Review标准:

    通常而言,只要代码对系统有明显的提升且正常工作,即便不完美,评审者也应该倾向于通过这次变更。

    这是所有Code Review指南中的高级原则。

    当然这也有些局限。例如,如果变更里加入了有些评审者在系统里不想要的功能,即便代码设计的很好,评审者也可以拒绝掉。

    没有完美无缺的代码,只有越来越好的代码。代码评审者绝不应该要求开发者打磨好CL中的每个细节才予以通过,相反,评审者应该权衡项目进度和他们给出建议的重要性,适当放宽要求。评审者应该追求 持续提高 ,而不是追求完美。那些可以提升整个系统可维护性、可读性和可以理解性的变更不应该因为代码不够完美而被推迟几天甚至几周。

    评审者要 始终 不拘谨于在代码评论里提示可以更好的想法。 但如果不是很重要信息,可以在评论前面加上标识告诉他们可以忽略。

    注意:这篇文档中没有任何地方辩解在变更中的检查会让整个系统代码变得 更糟糕 。你唯一能做的在紧急状况中说明。

    指导性

    Code Review有个重要的作用,那就是可以教会开发者关于语言、框架或者通用软件设计原理。在Code Review中留下评论来帮助开发者学习新东西是很值得提倡的,毕竟共享知识也是长期提升系统代码健康度的一部分。但请注意,如果你的评论纯粹是教育性的,并且不是这篇文档中提到的关键标准,请在前面加上“Nit:”标识,或者明确指出不需要在这次变更中解决。

    原则

    • 技术和数据高于意见和个人偏好。
    • 关于风格问题, 风格指南是绝对的权威。任何不在样式指南中指出的样式(比如空格等)都是个人偏好的问题。风格应该与现有的一致。如果没有以前的风格,就按作者的风格来。
    • 软件设计从来不是纯粹的代码风格或是个人偏好问题,它们是基于一些应当被权衡的规则而不仅仅是个人倾向。有时候也会有多种有效的选项,如果开发者能证明(通过数据或者原理)这些方法都同样有效,那么评审者应该接受作者的偏好,否则应该遵从软件设计标准。
    • 如果没有其他的规则使用,只要保证不会影响系统的健康度,评审者可以要求开发者保持和现有的代码库一致。

    解决代码冲突

    如果Code Review中有任何冲突,开发人员和评审人员都应该首先根据开发者指南评审者指南中其他文档的内容,尝试达成一致意见。

    当很难达成一致时,开发者和评审者不应该在Code Review评论里解决冲突,而是应该召开面对面会议或者找个权威的人来协商。(如果你在评论里协商,确保在评论里记录了讨论结果,以便日后其他人翻阅。)

    如果这样都解决不了问题,那解决问题的方式就应该升级了。通常的方式是拉着团队一起讨论、让团队主管来权衡、参考代码维护者的意见,或者让管理层来决定。不要因为开发者和评审者不能达成一致而把变更一直放在那里。

    Code Review应该关注什么?

    注意:当我们考虑以下点时,应当始终遵循Code Review标准

    设计

    Code Review中最重要的一个点就是把握住变更中的整体设计。变更中各个部分的代码交互是否正常?整个改动是否属于你负责的代码库?是否和你系统中其他部分交互正常?现在是否是添加整个功能的恰当时间?

    功能性

    开发者在这个变更中想做什么? 开发人员打算为该代码的用户带来什么好处?(这里”用户“是指受变更影响到的实际用户,和将来会使用到这些代码的开发者)

    重要的是,我们希望开发者能充分测试代码,以确保代码在Code Review期间正常运行。但无论如何,你作为审查者也要考虑一些特殊情况,像是有些并发问题。站在用户的角度,确保你正在看的代码没有bug。

    你可以验证变更,尤其是在有面向用户的影响时,评审者应该仔细检查整个变更。有时候单纯看代码很难理解这个变更如何影响到用户,这种情况下如果你不方便自己在CL中打补丁并亲自尝试,你可以让开发者为你提供一个功能性的demo。

    另一个在Code Review时需要特别关注的点是,CL中是否有 并发编程,并发理论上可能会导致死锁或资源争抢,这种问题在代码运行时很难被检测出来,所以需要有人(开发者和评审者)仔细考虑整个逻辑,以确保不会引入线程安全的问题。(所以除了死锁和资源争抢之外,增加Code Review复杂度也可以成为拒绝使用多线程模型的一个理由。)

    复杂性

    变更是否比预期的更复杂?检测变更的每个级别,是否个别行太复杂?功能是否太复杂?类是否太复杂?”复杂“意味着代码阅读者很难快速理解代码,也可意味着开发者尝试调用或修改此代码时可能会引入bug。

    一个典型的复杂性问题就是 过度设计,当开发者让代码变得更通用,或者增加了许多当前不需要的功能时,评审者应该额外注意是否过度设计。鼓励开发者解决现在遇到的问题,而不是揣测未来可能遇到的问题。未来的问题应当在遇到时解决,到那个时候你才能看到问题本质和实际需求。

    测试

    开发人员应当进行单元测试、集成测试或端到端测试。一般来说,变更中应该包含测试,除非这个变更只是为了处理紧急情况

    确保变更中的测试是正确、合理和有用的。因为测试本身无法测试自己,而且我们很少会为测试编写测试,所以必须确保测试是有效的。

    如果代码出了问题,测试会失败吗?如果代码发生改动,它们会误报吗?每一个测试都有断言吗?是否按照不同的测试方法对测试进行分类?

    记住,不要以为测试不是二进制中的一部分就不关注其复杂度。

    命名

    开发人员是否使用了良好的命名方式?好的命名要能够充分表达一个项(变量、类名等)是什么或者用来做什么,但又不至于让人难以阅读。

    注释

    开发者有没有写出清晰易懂的注释?所有的注释都是必要的吗? 通常注释应该解释清楚为什么这么做,而不是做了什么。如果代码不清晰,不能清楚地解释自己,那么代码可以写的更简单。当然也有些例外(比如正则表达式和复杂的算法,如果能够解释清楚他们在做什么,肯定会让阅读代码的人受益良多),但大多数注释应该指代码中没有包含的信息,比如代码背后的决策。

    变更中附带的其他注释也很重要,比如列出一个可以移除的待办事项,或者一个不要做出代码变更的建议,等等。

    注意,注释不同于类、模块或函数文档,文档是为了说明代码片段如何使用和使用时代码的行为。

    风格

    在谷歌内部,主流语言甚至部分非主流语言都有编码风格指南 ,确保变更遵循了这些风格规范。

    如果你想对风格指南中没有提及的风格做出改进,可以在注释前面加上“Nit:”,让开发人员知道这是一个你认为可以改进的地方,但不是强制性的。但请不要只是基于个人偏好来阻止变更的提交。

    开发人员不应该将风格变更与其他变更放在一起,这样很难看出变更里有哪些变化,让合并和回滚变得更加复杂,也会导致更多的其他问题。如果开发者想要重新格式化整个文件,让他们将重新格式化后的文件作为单独的变更,并将功能变更作为另一个变更。

    文档

    如果变更改变了用户构建、测试、交互或者发布代码相关的逻辑,检测是否也更新了相关文档,包括READMEs, g3doc页面,以及任何相关文档。如果开发者删除或者弃用某些代码,考虑是否也应该删除相关文档。如果文档有确实,让开发者补充。

    代码细节

    查看你整个Code Review中的每一行代码,比如你可以扫到的数据文件,生成的代码或大型数据结构,但不要只扫一遍手写的类,函数或者代码块,然后假设它们都能正常运行。显然,有些代码需要仔细检查,这完全取决于你,但你至少应该理解所有的代码都在做什么。

    如果你很难看懂代码,导致Code Review的速度慢了下来,你要让开发者知道,并且在Review前澄清原因。在谷歌,我们有最优秀的工程师,你也是其中之一。如果你都看不懂,很可能其他人也看不懂。所以要求开发者理清代码逻辑也是在帮助未来的开发者。

    如果你理解代码,但又觉得没有资格做代码评审,确保有资格的变更评审人员在代码评审时考虑到了安全性、并发性、可访问性、国际化等问题。

    上下文

    看改动上下文代码对Code Review很有帮助,因为通常Code Review工具只会显示改动部分上下几行代码,但有时你必须查看整个文件确保这次变更可以正常运行。例如,你可能看到加了4行代码,但是看到整个文件时才会发现这加的4行代码是在一个50多行的方法里,这个方法现在应该被拆分为多个小的方法了。

    Code Review时考虑到整个系统的上下文也很重要,这次改动提升了系统健康度?或者增加了系统复杂性?少了测试用例?…… 不要通过哪些会损害系统健康的代码。 很多系统变复杂都是因为一点一点的小改动日积月累造成的,所以千万不要放进来任何增加复杂性的改动。

    亮点

    如果你看到变更中做的好的地方,告诉开发者他们做的很棒,尤其是当他们用某种很精巧的方式解决你评论中提到的问题时更应不吝赞美。 Code Review过多关注于错误,但你也应该为开发者展现出好的一面点赞。在指导他人的时候,有时候告诉开发者正确的做法比告诉他们错误的做法更有价值。

    总结

    在做Code Review时,你需要注意以下:

    • 代码设计良好。
    • 代码功能对用户有用。
    • 任何UI改动应当是深思熟虑过而且看起来不错的。
    • 保证线程安全。
    • 代码没有增加不必要的复杂性。
    • 开发者没有写有些将来需要但现在不知道是否需要的东西。
    • 代码有适当的单元测试。
    • 测试逻辑设计良好。
    • 开发者使用了清晰明了的命名。
    • 注释清晰明了实用,通常解释清楚了为什么这么做,而不是做了啥
    • 代码又相应完善的文档。
    • 代码风格符合规范。

    确保你review了要求你看的每一行代码,确保你正在提升代码质量,并且为开发者做的提升点赞。

    Code Review步骤

    概要

    现在你知道了要从CL中得到什么,那能在众多文件中完成评审的方法是什么?

    1. 这条改动是否生效?这次变更是不是描述清晰?
    2. 首先阅读CL最重要的一部分,整体上是否设计良好?
    3. 按照合适的顺序阅读剩下的变更。

    第一步: 综观这个改动

    阅读CL描述并且明白CL大体内容。这些修改是否有意义?首先如果这个修改不应该有,请立刻说明为什么这些修改不应该有。当你因为这个拒绝了这次改动提交时,告诉开发人员该怎么去做是非常好的。

    例如,您可能会说:“看起来您为此做了一些出色的工作,谢谢!但是,我们实际上正着手删除FooWidget系统,您正在此处进行修改,因此我们不想进行任何新的修改现在。所以,您可以重构我们的新BarWidget类吗?"

    请注意,审阅者不仅拒绝了当前的CL并提供了替代建议,但他们礼貌地做到了。这种礼貌是重要,因为我们想表明我们甚至在开发人员之间也相互尊重,尤其是当我们意见不同时。

    如果您得到的多个CL并且您不想进行的更改,您应该考虑重新设计团队的开发流程或发布的外部贡献者的流程,以便在CL完成之前进行更多的交流。最好在做大量工作之前告诉人们“不必做”,因为现在要将其丢弃或彻底重写。

    第二步: 检查CL的主要部分

    查找属于此CL的“主要”部分的文件。通常来说一个CL最重要的部分是它逻辑修改数最多的那个文件。这样有助于了解相关的CL,通常来说会加快code review。如果CL太大,您无法确定哪些部分是主要部分,请开发人员告诉你应该首先看什么,或要求他们将CL拆分为多个CL

    如果您发现CL的这一部分存在一些主要的设计问题,则即使您现在没有时间审查CL的其余部分,也应立即写下这些评注。 实际上,检查其余的CL可能会浪费时间,因为如果设计问题足够严重,那么许多其他正在检查的代码将消失并且无论如何都不会发生。

    立即写下这些主要设计评注非常重要有两个主要原因:

    • 开发人员通常会发送给审核者一个CL,然后在等待审核时立即基于该CL进行新工作。 如果您正在审查的CL中存在重大设计问题,那么他们也将不得不重新设计其以后的CL。你能在他们做太多无用功之前制止他们。

    • 重要的设计变更比小的变更需要更长的时间。 开发人员几乎都有截止日期;为了在截止日期之前完成工作, 并在代码库中保留高质量的代码,开发人员需要尽快开始CL的所有主要的重做。

    第三步:依序阅读CL的其余部分

    确认CL整体上没有大的设计问题后,请尝试找出逻辑顺序来浏览文件,同时还要确保不要错过对任何文件的审查。通常,在浏览了主要文件之后,按照代码审查工具向您展示它们的顺序浏览每个文件是最简单的。有时在阅读主要代码之前先阅读测试代码也是有帮助的,因为这样您就可以知道更改应该做什么。

    Code Review的速度

    为什么Code Review应该快?

    在谷歌内部,我们在持续优化团队开发新产品的速度,而不是优化单个开发人员编写代码的速度。 个人开发的速度固然重要,但它没有团队整体的速度那么重要。

    如果Code Review速度慢了,可能会发生以下这些事:

    • 整个团队的速度会降低,是的,你不快速响应别人的Code Review也可以完成其他的工作。但是,整个团队的新功能、bug修复可能会因为没有人做cr被延迟几天、几周甚至几个月。
    • 开发者应维护Code Review的流程,如果审查者很少回复Code Review,但是每次都对CL提出大量的改动,这可能会打击到开发者。通常,开发者可能会抱怨审查者太严格。如果审查者能在开发者更新后快速响应,并提出有实质性提升的建议(能显著提升代码运行状况的CL),抱怨就会消失。 Code Review中绝大多数抱怨会随着CR速度的提升而消失。
    • 可能影响到代码质量。 如果CR慢了,可能会给开发者一种需要提交不太好代码的压力。CR慢了,也会影响到代码清理、重构、和现有CL的进一步提升。

    应该以什么样的速度做Code Review?

    如果你不是在做需要高度专注的任务,Code Review应该越快越好
    应当在一个工作日之内回应Code Review请求。
    遵循这些指导意味着典型的CL应该在一天内进行多轮审查(如果需要)。

    速度 vs 中断

    只有一种情况下,个人的速度要胜过团队速度。如果您正在处理诸如编写代码之类的重要工作,请不要打断自己的工作去做Code Review,研究人在专注中被打断后需要很长的时间才能重新恢复到专注状态。所以,为了不让其他开发者等会而且中断自己的编码工作,明显得不偿失。
    所以,建议你在工作断点的时候回应Code Review,比如写完代码、午饭后、会议结束后、从茶水间回来……

    快速响应

    当我们谈论Code Review的速度时,一方面是指响应时间,另一方面是指整个Review从提交到通过的时间。整个过程应该足够快,但是对于每个人来说,迅速做出反应比迅速完成整个过程更为重要。
    即使有时需要很长时间才能完成整个Code Review流程,评审者在整个过程中的快速响应也可以大大缓解开发人员因为Code Review“慢”而产生的挫败感。
    如果你太忙不能全身心投入到Code Review中,你也应该让开发者知道你什么时候会去Review,或者建议其他评审者快速响应,再或者提供一些初步的建议。(注意:这不是建议你中断自己的工作,而是在工作间隙合理响应)
    评审者有必要花时间去做Code Review来保证代码符合标准,不管怎么样,每个回应应当保证足够快速

    跨时区Code Review

    当遇到跨时区的Code Review时,尽量在作者回家前回复。如果他们已经回家了,尽量在他们每天来公司前完成Code Review。

    LGTM和注释

    为了让CR更快,有些情况下也应该让CR提前通过,即便有些评论没有被解决,比如:

    • 审查者信任开发者能适当解决所有评审者的建议。
    • 其余的改动很小,开发者不必做。

    除非另有明确说明,评审者应指明他们打算使用这些选项中的哪一个。
    当开发者和评审者在不同时区时,应当着重考虑下通过CR,否则开发者还得等一天。

    大的CL

    如果有人给你提交了一个非常大的Code Review,你也不确定你有时间看,你最好建议开发者把CL拆分成几个小的部分,分多次Code Review,而不是一次性全部提交上来。这即便开发者多做一些额外的工作,也是可以把它拆分开的,而且拆分也有利于评审者。

    如果CL不能拆分成多个小CL,你也没有时间快速完整的Review整个代码,只是对整体设计提一些建议,然后发回给开发者改进。作为评审者,你的目标之一是在不牺牲代码质量的前提下,不阻碍开发者的进程或者尽可能让他们向前推进。

    持续提升Code Review

    如果你遵从这些建议并在Code Review中严格执行这些准则,你就会发现整个Code Review的流程会越来越快。 开发者将了解健康代码的要求,并从一开始就交出完美的代码,然后Code Review的时间会越来越少。评审者将学会如何快速做出响应,并且不是在这个Code Review过程中增加不必要的延迟。
    但是,不要为了提升速度牺牲Code Review的标准和代码质量。 从长远来看,这并不会提升速度。

    紧急情况

    当然也有一些紧急的CL需要快速走完这个Code Review流程,这时候在质量上的把控可以稍微放松一些。可以参考紧急事件一文来了解哪些是紧急事件哪些不是。

    怎样写代码评论

    概要

    • 礼貌
    • 解释你的观点.
    • 明确指出方向和问题,帮助开发人员去权衡作出决定.
    • 鼓励开发人员通过注释和精简代码来解决你的困惑而不是通过解释

    礼貌

    通常来说当你Code Review代码时保持礼貌和尊重能使开发人员更加清晰,得到更多帮助。这样是为了保证你的代码评论仅仅针对的是code而不是针对开发人员。你不必一直这么去做,但是当你的评论会让开发人员生气或者产生争执时有必要这么去做。比如:

    不好的例子: “为什么会在这里使用线程,这样做难道会有任何好处?”

    好的例子: "我并没有发现这个并发模块给程序带来了多少帮助,并且还增加了程序的复杂性,因此我认为这段代码最好是用单线程而不是多线程。

    解释清楚原因

    从上面“好”的例子当中你能发现,这样有助于开发人员理解为什么你写了这些评注。你不一定非得包含这些信息在你的评注里面,但是适当的多解释你的意图或者多给出一些提升代码质量的建议都是非常好的实践。

    给予指导

    通常来说修复CL是开发人员的职责而不是评审人员的。你不需要向开发人员提供详细的解决方案或者代码。

    但是这并不意味着评审员就不应该提供帮助。你需要在指出问题和提供直接指导之间找到平衡。指出问题并且帮助开发人员决策能够帮助开发人员学同事让code review变得更加简单。这样也能产生更好的方案,因为开发人员比评审者更加了解代码。

    尽管这样,有时候直接给出指导,建议甚至是代码更有帮助。code review的主要目的是尽可能得到最好的CL。其次是提高开发人员的技能这样就能减少以后评审的次数。

    接受解释

    与其要求让开发人员解释一段你看不懂的代码,其实更应该做的是让他们重写代码,让代码更清晰。当一段代码不是太过于复杂的时候通过加一些注释偶尔也是一种不错的做法。

    **解释仅仅是写在Code Review工具里的,不利于将来的代码阅读者。**只有极少数情况是可行的,比如你对你评审的需求不太熟悉,但是开发人员解释的是大多数人都知道的。

    处理Code Review中的反驳

    有时开发者会在Code Review中反驳你,他们可能不同意你的意见,或者抱怨你太严格了。

    谁是谁非?

    当开发者不同意你的建议时,首先花点思考下他们是否是对的,但通常而言你比他们更熟悉代码,所以可能在某个方面理解更深。他们的争论有意义吗?从代码健康的角度来看他们的反驳有意义吗?如果是,让他们知道他们是对的,然后这个问题就解决了。

    然而,开发者不总是对的,这种情况下评审者应当进一步介绍为什么他们的建议是对的。好的解释不仅得体现出对开发人员回复的理解,而且还要说明为什么要这么改。

    如果评审者认为他们的建议可以改善代码质量,并且他们认为带来的代码质量改进值得开发者做这些额外的工作,评审者就应该坚持自己的立场。
    不积跬步无以至千里,不积小流无以成江河

    有时候要让开发者接受,你得花很多时间反复解释,但始终确保该有的礼貌,并让开发者知道在知道他们了什么,即便是你不同意。

    惹恼开发者

    有时评审者会认为坚持让开发者做出改动,可能会惹恼开发者。有时开发者确实会恼怒,但这种情况通常都很短暂,而且之后他们会感激你帮助他们提高了代码质量。 如果你在Code Review中很有礼貌,开发者根本不会被惹恼,这种担心是多余的。通常,令惹恼开发者的是你写注解的方式,而不是你对代码质量的坚持。

    稍后解决

    一种常见的反驳原因是开发者希望能尽快完成任务。他们不想一轮又一轮地做Code Review,然后就会说他们会在后续CL中处理这些问题,你只需要通过就行。有些开发者做的很好,他们会立马提交后续CL处理这些问题。然而,经验告诉我们原始CL通过之后拖的时间越久,就越不可能修复。除非开发者在当前CL通过后立马修复,否则他们就不可能修复。这并不是开发者不负责任,而是因为他们有好多工作要做,而修复工作也会因为工作压力而被遗忘。所以最好坚持让开发者现在就在CL中处理掉这些问题,“留着以后清理”是一种不可取的方式。

    如果CL中引入了新的复杂性,提交之前必须清理掉,除非是紧急情况。 如果CL中暴露出一些目前还无法定位的问题,开发者应该记录下这些bug,并将其分配给他们自己,确保这些问题不会被遗忘。他们还可以在代码中加入 TODO 注释,指向已经记录好的 bug。

    抱怨太严格

    如果你之前Code Review很宽容,然后突然变得严格起来,可能会引起一些开发者的抱怨。不过没关系,加快Code Review的速度通常会让这些抱怨消失。

    有时,这些抱怨可能需要几个月的时间才能消除,但最终开发者在看到产出的优质代码时会理解严格Code Review带来的价值。有时候,一旦发生某些事让他们真正看到严格Code Review的价值,抗议最大声的人甚至会成为你最坚定的支持者。

    解决冲突

    如果你遵循了上述方法,但仍然会在Code Review中遇到无法解决的冲突,请再次参阅Code Review标准,了解那些有助于解决冲突的指导和原则。

    开发者指南

    写一个好的CL描述

    一个CL描述是做了什么更改以及为什么的公开记录。
    它将成为我们版本控制历史的永久组成部分,多年来除你的评审者以外,还可能有数百人会阅读它。
    将来的开发者将会基于它的描述来搜索你的CL。
    将来可能会有人由于没有现成的细节,而根据他模糊的记忆来寻找你的改变。
    如果所有重要的细节都在代码中而不是描述中,那么他们将很难定位你的CL。

    第一行

    • 言简意赅
    • 语义完整
    • 空行隔开

    CL描述的第一行应该是对CL正在做的具体工作的简短总结,紧跟一个空行。
    在未来,这是大多数的代码搜索者在浏览一段代码的版本控制历史时会看到的,因此第一行应该足够有信息量,他们不必阅读你的CL或它的整个描述就可以大致了解你的CL实际做了什么。
    按照传统,CL描述的第一行是一个完整的句子,就像它是一个命令(一个祈使句)。
    例如,说"Delete the FizzBuzz RPC and replace it with the new system.",而不是"Deleting the FizzBuzz RPC and replacing it with the new system."。
    不过,你不必把其余的描述写成祈使句。

    正文内容丰富

    其余描述应该是具有丰富的内容。它可能包括对正在解决的问题的简要描述,以及为什么这是最好的方法。 如果这种方法有任何缺点,就应该提到。将相关的背景信息(例如bug数目,基准测试结果),以及设计文档的链接。即使是很小的CL也值得关注细节。请将来龙去脉放入CL中。

    反例

    "Fix bug"是一个不充分的CL描述。是什么bug?你是怎么修复它的?
    其他一些类似的不好的CL描述:

    • “Fix build.”
    • “Add patch.”
    • “Moving code from A to B.”
    • “Phase 1.”
    • “Add convenience functions.”
    • “kill weird URLs.”

    这里有一些是真实的CL描述。他们的作者可能认为他们提供的信息是有用的,但他们并没有给出一个CL描述的意图。

    好的CL描述

    这里有一些好的CL描述的例子.

    功能改变

    rpc: remove size limit on RPC server message freelist.

    Servers like FizzBuzz have very large messages and would benefit from reuse.
    Make the freelist larger, and add a goroutine that frees the freelist entries
    slowly over time, so that idle servers eventually release all freelist
    entries.

    前几个词描述了CL描述实际做了什么。其余描述在说明解决的问题,为什么这是一个好的方案,以及具体实现的细节。

    重构

    Construct a Task with a TimeKeeper to use its TimeStr and Now methods.

    Add a Now method to Task, so the borglet() getter method can be removed (which
    was only used by OOMCandidate to call borglet’s Now method). This replaces the
    methods on Borglet that delegate to a TimeKeeper.

    Allowing Tasks to supply Now is a step toward eliminating the dependency on
    Borglet. Eventually, collaborators that depend on getting Now from the Task
    should be changed to use a TimeKeeper directly, but this has been an
    accommodation to refactoring in small steps.

    Continuing the long-range goal of refactoring the Borglet Hierarchy.

    第一行描述了CL做了什么,以及它是如何与过去发生变化的。
    其余的描述将讨论具体的实现、CL的来龙去脉、解决方案的不理想以及未来可能的方向。
    它同样是解释为什么要进行这个修改。

    需要上下文的小CL

    Create a Python3 build rule for status.py.

    This allows consumers who are already using this as in Python3 to depend on a
    rule that is next to the original status build rule instead of somewhere in
    their own tree. It encourages new consumers to use Python3 if they can,
    instead of Python2, and significantly simplifies some automated build file
    refactoring tools being worked on currently.

    第一句描述了实际做了什么。其余的描述解释了为什么要进行更改,并给了reviewer很多背景。

    在提交CL之前,请检查描述

    在Review期间CL可能会经历重大改变。在提交CL之前,有必要review CL描述,以确保描述仍然反映CL所做的事情。

    简短化的CL

    为什么要写成一系列简短的CL?

    简短的CL有这些好处:

    • Code Review更快 与比起花30分钟审查一个大型的CL相比,对审查者来说花5分钟审查一系列小的CL更加容易.
    • 审查更加彻底。 进行较大的更改后,审阅者和作者往往会因大量详细评论的来回移动而感到沮丧,有时这些评论会漏掉或遗漏重要的观点。
    • 减少导致bug的可能性。 由于您所做的更改较少,因此您和您的审阅者更容易有效地推断出CL的影响,并查看是否导致bug。
    • 减少不必要的工作 当你写了一个巨大的CL,然后审查者觉得你总体方向错了,这会导致你浪费大量的工作。
    • 更方便合并代码 因为大型的CL会导致大量的冲突,因此合并大型的CL会浪费很多时间,并且这将会是你经常做的工作。
    • 有助于你作出更好的设计 优雅的设计并且完善一个小的改动比大的改动更加容易
    • 降低审查者的难度 提审部分改动,不会影响你继续编码。
    • 回滚更容易 大型CL很有可能会在初始CL提交和回滚CL之间更新修改文件,从而使回滚变得复杂(中型CL也可能也需要回滚可能也会这样)。

    注意! 审查者可以因为你的改动过于巨大直接拒绝掉你通常,他们会感谢您的贡献,但要求您以某种方式使它成为一系列较小的更改。不管是你把这些改动拆分成小的改动,还是和审查者争论让他接受都会耗费你大量的时间。但是编写小型改动就不会有这样的问题。

    怎么样算简短?

    通常,CL的正确大小是一个独立的更改。 这意味着:

    • CL所做的最小更改仅解决了一件事情。 通常,这只是功能的一部分,而不是一次完整的功能。 通常,宁可编写过小的CL也不要写太大的CL。你可以和你的审查者商量找到合适的尺度。
    • 审阅者需要了解的有关CL的所有内容(将来的开发除外)都在CL,CL的描述,现有代码库或他们已经查看过的CL中。
    • 检入CL后,系统将继续对其用户和开发人员正常运行。
    • CL不够小的话会导致其难以理解。如果你新增加了api,那么在同一个CL应该包括这个api用到的方法,以便审查者更好的理解。也能防止检入没有用的api。

    多大算大,没有一个明确的要求。 对于CL而言,100行通常是一个合理的大小,而1000行通常太大,但这取决于您的审阅者的判断。 更改分布的文件数量也会影响其“大小”。 可以在一个文件中进行200行的更改,但是将其分布在50个文件中通常会太大。

    请记住,尽管从您开始编写代码起就一直与您紧密联系,但审阅者通常没有上下文。 对您来说,看起来像可接受大小的CL可能会让您的审阅者不知所措。毫无疑问,尽可能把CL些小是正确的。审查者很少抱怨CL太小

    什么时候大型的CL也是可以的?

    在某些情况下,较大的更改没有那么糟糕:

    • 通常,您可以将整个文件的删除视为更改的一行,因为它不会花费很长时间来审阅审阅者。
    • 有时,您完全信任的自动重构工具已经生成了一个较大的CL,而审阅者的工作只是检查健全性,然后指出他们认为需要修改的地方。 这些CL可能更大,尽管会有一些风险(例如合并和测试)仍然适用。

    按文件分类

    拆分CL的另一种方法是通过将文件分组,如果这些文件将是独立的更改,可以分配各不同的审阅者。

    比如: 你提交一个修改的CL,创建一个修改的缓冲区,另一个CL的代码修改也可以提交到这里面。你必须按照顺序提交CL,但是审阅者可以同时进行审阅. 如果这么做,你需要尽可能告诉所有审阅者另一个CL的信息,以便他们能得到上线文信息。

    另一个示例:您发送一个CL进行代码更改,而另一个CL则发送使用该代码的配置或实验; 如果需要,这也更容易回滚,因为有时将配置/实验文件推送到生产环境中比更改代码更快。

    把代码重构分离出来

    通常重构最好不要和功能修改或者bug fix一起提CL。比如移动或者重命名一个Class,最好和这个Class的bug fix分开提CL。这样对于审阅者来说,理解每个CL单独引入的更改要容易得多。

    不过一些小的代码清理工作比如变量重命名可以包含在功能修改或者bug fix的CL种。 这取决于开发者和审阅者的判断,当然如果巨大的重构包含在同一个CL中会大大增加审阅的难度。

    将相关的测试代码保存在同一CL中

    避免将测试代码拆分为单独的CL。 验证代码修改的测试应该进入相同的CL,即使这样做会增加代码行数。

    但是根据重构准则独立的测试修改可以单独写入CL。比如

    • 使用新测试验证先前存在的提交代码。
    • 重构测试代码(例如,引入辅助函数)。
    • 引入更大的测试框架代码(例如集成测试)。

    不要破坏结构

    如果您有多个相互依赖的CL,则需要找到一种方法来确保在提交每个CL之后整个系统都能正常工作。 否则,可能破坏代码结构导致后面的开发者浪费大量的时间等你的提交(如果这些CL提交出了问题,则更长的时间)。

    小到不能再小

    有时候,您会遇到CL很大的情况。 这很少是真的。 练习编写小型CL的作者几乎总是可以找到一种将功能分解为一系列小的更改的方法。

    在写大型CL之前,思考下是不是能够将重构分离出来,这是一个更加清晰的思路。多和你的队友交流学习下他们对缩小CL的实践和方法。

    如果所有这些选项都失败了(这种情况很少见),那么请事先获得您的审阅者的同意,告诉他们一个巨大的CL将要来临。 在这种情况下,审查过程会耗费极其长的实践,这样请花费更多的时间去写测试代码,避免引入bug。

    如何处理评审者的评论

    当你提交了一个变更做Code Review时,很可能你都会收到评审者在变更中的评论。这里有一些处理这些评论的建议。

    不要掺杂个人情感

    Code Review的目标是维护代码库和产品的质量。如果评审者批评了你的代码,可以理解为他们在帮你、帮整个代码库、甚至是帮整个公司,而不是攻击你或者是质疑你的能力。

    有时候评审者会情绪低落,然后在评论中说出一些令人沮丧的话,虽然评审者这样做不对,但作为开发者你应当有心理准备。问问你自己,“评审者试图与我交流的建设性意见是什么?” 然后照他们说的那些去做。

    永远不要回应充满怒气的评论,Code Review工具中违反职业礼仪的情况永远存在。如果你真的忍无可忍,建议先离开电脑也会儿,或者干一些其他的事,等心情平复下来再回复。

    通常,如果评审者没有以礼貌的方式提供反馈,请亲自向他们解释。如果你无法亲自或者通过视频和他们联系,就向他们发一份私人邮件。 友好地告诉他们你不喜欢的事情以及你希望他们做些什么。 如果他们也以非建设性的方式对此私密讨论做出回应,或者没有起到预期的作用,请酌情上报给您的经理。如果他们已经以不礼貌的方式回应,或者没有取得预期的效果,视情况汇报给你的经理。

    修复代码

    如果评审者说他们理解不了你代码中的某些内容,你首先应该把代码写清晰。如果让代码更清晰,添加注释来解释清楚代码的逻辑。 如果评论似乎毫无意义,那么您的答复应该只是代码查看工具中的解释。

    如果评审者无法理解你的某部分代码,那边可能未来的阅读者也可能理解不了。在Code Review工具中回应帮不了未来的读者,但是代码中的注释可以。

    自我反思

    写一个变更会花费你很大的精力,提价Code Review时会感觉如释负重,而且自己也相当确定所有工作已经做完了。所以当评审者提出改进建议时,你很容易认为那些都是错的,或者认为是评审者给你不必要的阻挠,再或者觉得评审者应该让你提价变更。 无论如何,不管你怎么想,花点时间回想下评审者给你的反馈有助于提升公司的代码质量。你始终问下自己“如果评审者是对的呢?”

    如果你回答不了评审者的问题,那可能说明评审者的评论不够清楚。

    如果你认真考虑过后依旧认为你是对的,放心大胆地解释清楚为什么你的方法对公司更有利。通常,评审者只是提供建议,并且希望你能思考出更好的方法。也许你已经知道一些评审者不知道的关于用户、代码库、或者变更,把这些都写下来,给评审者更多的上下文信息,通常你都可以根据某些事实和评审者达成某些共识。

    解决冲突

    解决冲突的第一步,和你的评审者达成共识,如果无法达成共识,参阅Code Review的标准获取更多内容。

    展开全文
  • 在 softpedia 上了解我们:http://www.softpedia.com/get/Desktop-Enhancements/Clocks-Time-Management/Timer-review.shtml
  • 你有 Code Review 吗?.pdf
  • 从Code Review 谈如何技术

    千次阅读 2014-09-18 16:16:13
    因为翻看了阿里内部的Review Board上的记录,从上面发现Code Review做得好的是一些比较偏技术的团队,而偏业务的技术团队基本上没有看到Code Review的记录。当然,这并不能说没有记录他们就没有做Code Review,于是...

    (这篇文章缘由我的微博,我想多说一些,有些杂乱,想到哪写到哪)

    这两天,在微博上表达了一下Code Review的重要性。因为翻看了阿里内部的Review Board上的记录,从上面发现Code Review做得好的是一些比较偏技术的团队,而偏业务的技术团队基本上没有看到Code Review的记录。当然,这并不能说没有记录他们就没有做Code Review,于是,我就问了一下以前在业务团队做过的同事有没有Code Review,他告诉我不但没有Code Review,而且他认为Code Review没用,因为:

    1)工期压得太紧,时间连coding都不够,以上线为目的,

    2)需求老变,代码的生命周期太短。所以,写好的代码没有任何意义,烂就烂吧,反正与绩效无关。

    我心里非常不认同这样的观点,我觉得我是程序员,我是工程师,就像医生一样,不是把病人医好就好了,还要对病人的长期健康负责。对于常见病,要很快地医好病人很简单,下猛药,大量使用抗生素,好得飞快。但大家都知道,这明显是“饮鸩止渴”、“竭泽而渔”的做法。医生需要有责任心和医德,我也觉得程序员工程师也要有相应的责任心和相应的修养。东西交给我我必需要负责,我觉得这种负责和修养不是”做出来“就了事了,而是要到“做漂亮”这个级别,这就是“山寨”和“工业”的差别。而只以“做出来”为目的标准,我只能以为,这样的做法只不过是“按部就班”的堆砌代码罢了,和劳动密集型的“装配生产线”和“砌砖头”没有什么差别,在这种环境里呆着还不如离开。

    老实说,因为去年我在业务团队的时候,我的团队也没有做Code Review,原因是多样的。其中一个重要原因是,我刚来阿里,所以,需要做的是在适应阿里的文化,任何公司都有自己的风格和特点,任何公司的做法都有他的理由和成因,对于我这样的一个初来者,首要的是要适应和观察,不要对团队做太多的改动,跟从、理解和信任是融入的关键。(注:在建北京团队和不要专职的测试人员上我都受到了一些阻力),所以跟着团队走没有玩Code Review。干了一年后,觉得我妥协了很多我以前所坚持的东西,觉得自己的标准在降低,想一想后背拔凉拔凉的,所以我决定坚持,而且还要坚持高标准。

    对于Code Review很重要的这个观点,在微博上抛出来后,被一些阿里的工程师,架构师/专家,甚至资深架构师批评,我在和他们回复和讨论的过程中,居然发现有个“因为对方用户的设置”我无法回复了(我被拉黑了,还有一些直接就是冷讽和骂人了,微博中我就直接删除了)。这些批评我的阿里工程师/架构师的观点总结一下如下:(顺便说一下,阿里内还是有很多团队坚持做Code Review的

    1)到业务团队体会一下,倒逼工期的项目有多少?订好交付日期后再要求提前1个月的有多少?现在是做到已经不容易,更不谈做得漂亮!。

    2)Code Review是一种教条,意义不大,有测试,只要不出错,就可以了。

    3)目标都是改进质量,有限的投入总希望能有最大的产出,不同沉湎改进质量的方式不一样,业务应用开发忙的跟狗一样,而且业务逻辑变化快,通用性差,codereviw的成本要比底层高。

    4)现在的主要矛盾是倒排出来的工期和不靠谱的程序员之间的矛盾,我认为cr不是解决这个问题的银弹。不从实际情况出发光打正义的嘴炮实在太过于自慰了 。

    我们可以看到,上面观点其实和Code Review没有太多关系,其实是在抱怨另外的问题。这些观点其实是技术团队和业务团队的矛盾,但不知道为什么强加给了我的“Code Review很重要”的这个观点,然后这些观点反过来冲击“Code Reivew”,并说“Code Review无用”。这种讨论问题的方式在很常见,你说A,我说B,本来A、B是两件事,但就是要混为一谈,然后似是而非的用B来证明你的A观点是错的。(也许,这些工程师/架构师心存怨气,需要一个发泄的通道)

    我觉得,很多时候,人思考问题思考不清楚,很大一部分原因是因为把很多问题混为一谈,连我自己有些时候都会这样。引以为戒。

    即然被混为一谈,那我就来拆分一下,也是下面这三个问题:

    • Code Review有没有用的问题。
    • Code Review做不起来的问题。
    • 业务变化快,速度快的问题,技术疲于跟命。

    Code Review

    你Google一下Code Reivew这个关键词,你就会发现Code Review的好处基本上是不存在争议的,有很多很多的文章和博文都在说Code Review的重要性,怎么做会更好,而且很多公司在面试过程中会加入“Code Review”的问题。打开Wikipedia的词条你会看到这样的描述——

    卡珀斯·琼斯(Capers Jones)分析了超过12,000个软件开发项目,其中使用正式代码审查的项目,发现潜在缺陷率约在60-65%之间,若是非正式的代码审查,发现潜在缺陷率不到50%。大部份的测试,发现的潜在缺陷率会在30%左右。

    对于一些关键的软件(例如安全关键系统的嵌入式软件),一般的代码审查速度约是一小时150行程序码,一小时审查数百行程序码的审查速度太快,可能无法找到程序中的问题。代码审查一般可以找到及移除约65%的错误,最高可以到85%。

    也有研究针对代码审查找到的缺陷类型进行分析。代码审查找到的缺陷中,有75%是和计算机安全隐患有关。对于产品生命周期很长的软件公司而言,代码审查是很有效的工具。

    Code Review的好处我觉得不用多说了,主要是让你的代码可以更好的组织起来,有更易读,有更高的维护性,同时可以达到知识共享,找到bug只是其中的副产品。这个东西已经不新鲜了,你上网可以找到很多文章,我就不多说了。就像你写程序要判断错误一样,Code Review也是最基本的常识性的东西。

    我从2002年开始就浸泡在严格的Code Review中,我的个人成长和Code Review有很大的关系,如果我的成长过程中没有经历过Code Review这个事,我完全不敢想像。

    我个人认为代码有这几种级别:1)可编译,2)可运行,3)可测试,4)可读,5)可维护,6)可重用。通过自动化测试的代码只能达到第3)级,而通过Code Review的代码少会在第4)级甚至更高。关于Code Review,你可以参看本站的《Code Review中的几个提示

    可见,Code Review直接关系到了你的工程能力!

    Code Review 的问题

    有下面几个情况会让你的Code Review没有效果。

    首当其冲的是——“人员能力不足”,我经历过这样的情况,Code Review的过程中,大家大眼瞪小眼,没有什么好的想法,不知道什么是好的代码,什么是不好的代码。导致Code Review大多数都在代码风格上。今天,我告诉你,代码风格这种事,是每个程序员自查的事情,不应该浪费大家的时间。对此,我有两个建议:1)你团队的人招错了,该换血了。2)让你团队的人花时候阅读一下《代码大全》这本书(当然,还要读很多基础知识的书)。

    次当其冲的是——“结果更重要”,也就是说,做出来更重要,做漂亮不重要。因为我的KPI和年终奖based on how many works I’ve done!而不是How perfect they are ! 这让我想到那些天天在用Spring MVC 做CRUD网页的工程师,我承认,他们很熟练。大量的重复劳动。其实,仔细想一下好多东西是可以框架化,模板化,或是自动生成的。所以,为了堆出这么多网页就停地去堆,做的东西是很多,但是没有任何成长。急功近利,也许,你做得多,拿到了不错的年终奖,但是你失去的也多,失去了成为一个卓越工程师的机会。你本来可以让你的月薪在1-2年后翻1-2倍的,但一年后你只拿到了为数不多的年终奖。

    然后是——“人员的态度问题”,一方面就是懒,不想精益求精,只要干完活交差了事。对此,你更要大力开展Code Review了,让这种人写出来的代码曝光在更多人面前,让他为质量不好的代码蒙羞。另一方面,有人会觉得那是别人的模块,我不懂,也没时间 去懂,不懂他的业务怎么做Code Review? 我只想说,如果你的团队里这样的“各个自扫门前雪”的事越多,那么这个团队也就越没主动性,没有主动性也就越不可能是个好团队,做的东西也不可能好。而对于个人来说,也就越不可能有成长。

    接下来是——“需求变化的问题”,有人认识,需求变得快,代码的生存周期比较短,不需要好的代码,反正过两天这些代码就会被废弃了。如果是一次性的东西,的确质量不需要太高,反正用了就扔。但是,我觉得多多少少要Review一下这个一次性的烂代码不会影响那些长期在用的代码吧,如果你的项目全部都是临时代码,那么你团队是不是也是一个临时团队?关于如果应对需求变化,你可以看看本站的《需求变化与IoC》《Unix的设计思想来应对多变的需求》的文章 ,从这些文章中,我相信你可以看到对于需求变化的代码质量需要的更高。

    最后是——“时间不够问题”,如果是业务逼得紧,让你疲于奔命,那么这不是Code Review好不好问题,这是需求管理和项目管理的问题以及别的非技术的问题。下面我会说。

    不管怎么样,上述Code Review的问题不应该成为“Code Review无意义”的理由或借口,这就好像“因噎废食”一样。干什么事都会有困难和问题的,有的人就这样退缩了,但有的人看得到利大于弊,还是去坚持,人与人的不同正在这个地方。这就是为什么运动会受伤,但还是会人去运动,而有人因为怕受伤就退缩了一样。

    被业务逼得太紧

    被业务逼得太紧,需求乱变,这其实和Code Review没有多大关系了。对此,我想先讲一个我的故事。

    我去年在阿里的聚石塔,刚去的时候,聚石塔正在做一个很大的重构——对架构的大调整。因此压了很多业务需求,等这个项目花了2-3个月做完了后,一下子涌入了30-50个需求,还规定一个月完成,搞得团队疲于奔命。在累了两周后,我仔细分析了一下这些需求,发现很多需求是在重复做阿里云已经做过的东西,还有一些需求是因为聚石塔这个平台不规范没有标准所产生的问题。于是,我做了这么三件事:

    1)重新定义聚石塔这个产品主要目标和范围,确定哪些该做,哪些不该做。

    2)为聚石塔制定标准 ,让阿里云的API都长得基本一样,并制订云资源的接入标准。

    3)推动重构阿里云的Portal系统,不再实现阿里云已经做过的东西,与阿里云紧密结合。

    这些事情推动起来并不容易,聚石塔的业务方一开始也不理解,我和产品一起做业务方的工作,而阿里云也被我逼得很惨(在这里一并感谢,尤其阿里云的同学,老实说,和阿里云跨团队合作中是我这么多年来感觉最好的一次,相当赞)。通过这个事,聚石塔需求一下就有质的下降了。搞得还有几个工程师来和我说,你这么搞,聚石塔就没事可干了。姑且不说工程师对聚石塔的理解是怎么样的。 我只想说,我大量地减少了需求,尽最大可能联合了该联合的人,而不是自己闭门造车,并让产品的目标和方向更明确了。做了这些事情后,大家不但不用加班,而且还有时间充电去学技术,并为聚石塔思考未来的方向和发展。去年公司996的时候,我的团队还在965(搞得跟异教徒似的),而且还有很多时间去专研新的东西。

    说这个故事,我不是为了得瑟,而是因为有些人在微博上抨击我是一个道貌岸然的只会谈概念讲道理的装逼犯。所以,我告诉大家我在聚石塔是怎么做的,我公开写在这里,你也可以向相关的同学去求证我说的是不是真的。也向你证明,我可能是个装逼犯,但绝不是只会谈概念讲道理的装逼犯。

    被业务方逼得紧不要抱怨,你没有时间被逼得像牲口一样工作,这个时候,你需要的是暂停一下想一想,为什么会像牲口一样?而这正是让你变得聪明的机会。

    我为你总结一下,

    1)你有没有去Review业务部门给你的这么多的需求,哪些是合理的,哪些是不合理的。在Amazon,开发工程师都会被教育拿到需求后一定要问——“为什么要做?业务影响度有多大?有多少用户受益?”,回答不清这个问题,没有数据的支持,就不做。所以,产品经理要做很多数据挖拙和用户调研的工作,而不是拍拍脑袋,听极少数的用户抱怨就要开需求了。

    2)产品经理也要管理和教育的。你要告诉你的产品经理:“你是一个好的产品经理,因为你不但对用户把握得很好,也会对软件工艺把握得很好。你不但会开出外在的功能性需求,也同样会开出内在的让软件系统更完善的非功能性需求。你不是在迁就用户,而是引导用户。你不会无限制地加功能,而是把握产品灵魂控制并简化功能。你会为自己要做的和不做东西的感到同样的自豪。”你要告诉你的产品经理:“做一个半成品不如做好半年产品”(更多这样的观点请参看《Rework摘录和感想》)

    3)做事情是要讲效率的。Amazon里喜欢使用一种叫T-Shirt Size Estimation的评估方法来优先做投入小产出大的“Happy Case”。关于什么是效率,什么是T-Shirt Size Estimation,你可以看看《加班与效率》一文 。

    4)需求总是会变化的,不要抱怨需求变化太快。你应该抱怨的是为什么我们没有把握好方向?老变?这个事就像踢足球一样,你要去的地方是球将要去的地方,而不是球现在的地方。你要知道球要去哪里,你就知道球之前是怎么动的,找到了运动轨迹后,你才知道球要去像何方。如果你都不知道球要去向何方,那你就是一只无头苍蝇一样,东一下西一下。

    当你忙得跟牲口一样,你应该停下来,问一下自己,自己成为牲口的原因,是不是就是因为自己做事时候像就牲口一样思考?

    其它

    最后,我在给阿里今年新入职的毕业生的“技塑人生”的分享中,我给他们布置了5、6个Homework,分享几个给大家:

    1)重构或写一个模块,把他做成真正的Elegant级别。

    2)与大家分享一篇或几篇技术文章 ,并收获10-30个赞。

    3)降低现有至少20%的重复工作或维护工作

    4)拒绝或简化一个需求(需要项目中所有的Stakeholders都同意)

    部署这些作业的原因,是我希望新入行的同学们对自己的工作坚持高的标准,我知道你们会因为骨感的现实而妥协,但是我希望你们就算在现实中妥协了也要在内心中坚持尽可能高的标准,不要习惯成自然,最后被社会这个大染缸给潜移默化了。因为你至少要对自己负责。对自己负责就是,用脚投票,如果妥协得受不了了就离开吧

    芝兰生于空谷,不以无人而不芳!君子修身养道,不以穷困而改志!

    谢谢听我唠叨。

    转载自:http://coolshell.cn/articles/11432.html

    展开全文
  • review-react:评论React教程
  • 大家是怎么Code Review的?.pdf
  • 如何code review

    千次阅读 2008-12-05 09:55:00
    摘 要Code Review是一种通过复查代码提高代码质量的过程,在XP方法中占有极为重要的地位,也已经成为软件工程中一个不可缺少的环节。本文通过对Code Review的一些概念和经验的探讨,就如何进行Code Review和Code ...

    摘 要

    Code Review是一种通过复查代码提高代码质量的过程,在XP方法中占有极为重要的地位,也已经成为软件工程中一个不可缺少的环节。本文通过对Code Review的一些概念和经验的探讨,就如何进行Code Review和Code Review中应该注意什么提出一些建议。 本文中涉及的问题大部分针对JAVA类代码。同时本文不涉及Code Review过程和组织。

    关键词: Code Review,JAVA, XP ,代码质量 ,软件工程

    一、Code Review简介
    1 Code Review的目的
    凡事知其然还要知其所以然,我们首先需要知道什么是Code Review和我们使用它的目的是什么。Code Review是一种用来确认方案设计和代码实现的质量保证机制,通过这个机制我们可以对代码,测试过程和注释进行检查。Code Review主要用来在软件工程过程中改进代码质量,通过Code Review可以达到如下目的:

    在项目早期就能够发现代码中的BUG

    帮助初级开发人员学习高级开发人员的经验,达到知识共享

    避免开发人员犯一些很常见,很普通的错误

         保证项目组人员的良好沟通

         项目或产品的代码更容易维护

    2 Code Review的前提
    知道了Code Review的目的,我们就可以看看如何做Code Review了,但在做Code Review前我们还有事要做,所谓预则立,不预则废,就是说如果在进入Code Review之前我们不做些准备工作,Code Review很容易就变得没有意义或是流于形式,这在我们周围是有很多例子的啊。进入Code Review需要检查的条件如下:

    a)        Code Review人员是否理解了Code Review的概念和Code Review将做什么

    如果做Code Review的人员不能理解Code Review对项目成败和代码质量的重要程度,他们的做法可能就会是应付了事。

    b)       代码是否已经正确的build,build的目的使得代码已经不存在基本语法错误

    我们总不希望高级开发人员或是主管将时间浪费在检查连编译都通不过的代码上吧。

    c)       代码执行时功能是否正确

    Code Review人员也不负责检查代码的功能是否正确,也就是说,需要复查的代码必须由开发人员或质量人员负责该代码的功能的正确性。

    d)       Review人员是否理解了代码

    做复查的人员需要对该代码有一个基本的了解,其功能是什么,是拿一方面的代码,涉及到数据库或是通讯,这样才能采取针对性的检查

    e)        开发人员是否对代码做了单元测试

    这一点也是为了保证Code Review前一些语法和功能问题已经得到解决,Code Review人员可以将精力集中在代码的质量上。

    3 Code Review需要做什么
    好了,进入条件准备好了,有人在这些条件中看到Code Review这也不负责,那也不检查,不禁会问,Code Review到底做什么?其实Code Review主要检查代码中是否存在以下方面问题:代码的一致性、编码风格、代码的安全问题、代码冗余、是否正确设计以满足需求(性能、功能等等),下边我们一一道来。以下内容参考了《Software Quality Assurance: Documentation and Reviews》一文中的代码检查部分。

    3.1完整性检查(Completeness)
    代码是否完全实现了设计文档中提出的功能需求

    代码是否已按照设计文档进行了集成和Debug

    代码是否已创建了需要的数据库,包括正确的初始化数据

    代码中是否存在任何没有定义或没有引用到的变量、常数或数据类型

    3.2一致性检查(Consistency)
    代码的逻辑是否符合设计文档

    代码中使用的格式、符号、结构等风格是否保持一致

    3.3正确性检查(Correctness)
            代码是否符合制定的标准

            所有的变量都被正确定义和使用

            所有的注释都是准确的

            所有的程序调用都使用了正确的参数个数

    3.4可修改性检查(Modifiability)
            代码涉及到的常量是否易于修改(如使用配置、定义为类常量、使用专门的常量类等)

            代码中是否包含了交叉说明或数据字典,以描述程序是如何对变量和常量进行访问的

            代码是否只有一个出口和一个入口(严重的异常处理除外)

    3.5可预测性检查(Predictability)
            代码所用的开发语言是否具有定义良好的语法和语义

            是否代码避免了依赖于开发语言缺省提供的功能

            代码是否无意中陷入了死循环

            代码是否是否避免了无穷递归

    3.6健壮性检查(Robustness)
            代码是否采取措施避免运行时错误(如数组边界溢出、被零除、值越界、堆栈溢出等)

    3.7结构性检查(Structuredness)
            程序的每个功能是否都作为一个可辩识的代码块存在

            循环是否只有一个入口

    3.8可追溯性检查(Traceability)
            代码是否对每个程序进行了唯一标识

            是否有一个交叉引用的框架可以用来在代码和开发文档之间相互对应

            代码是否包括一个修订历史记录,记录中对代码的修改和原因都有记录

            是否所有的安全功能都有标识

    3.9可理解性检查(Understandability)
            注释是否足够清晰的描述每个子程序

            是否使用到不明确或不必要的复杂代码,它们是否被清楚的注释

            使用一些统一的格式化技巧(如缩进、空白等)用来增强代码的清晰度

            是否在定义命名规则时采用了便于记忆,反映类型等方法

            每个变量都定义了合法的取值范围

            代码中的算法是否符合开发文档中描述的数学模型

    3.10可验证性检查(Verifiability)
            代码中的实现技术是否便于测试

    二、Code Review经验检查项
    以下是在实践中建立的检查列表(checklist),通过分类和有针对性的检查项,保证了Code Review可以有的放矢。

    1 JAVA编码规范方面检查项
    检查项参照JAVA编码规范执行,见《JAVA编码规范(Java Code Conventions)》

    2 面向对象设计方面检查项
    这几点的范围都很大,不可能在本文展开讨论,有专门的书籍介绍这方面问题,当然在Code Review中主要靠经验来判断。

    A)        类设计和抽象是否合适

    B)         是否符合面向接口编程的思想

    C)        是否采用合适的设计范式

    3 性能方面检查项
    性能检查在大多数代码中都是需要严重关注的方面,也是最容易出现问题的方面,常常有程序员写出了功能和语法没有丝毫问题的代码后,正式运行时却在性能上表现不佳,从而不得不做大量的返工,甚至是推倒重来。

    A)        在海量数据出现时,队列,表,文件,在传输,upload等方面是否会出现问题,有无控制,如分配的内存块大小,队列长度等控制参数

    B)        对hashtable,vector等集合类数据结构的选择和设置是否合适,如正确设置capacity,load factor等参数,数据结构的是否是同步的

    C)        有无滥用String对象的现象

    D)        是否采用通用的线程池、对象池模块等cache技术以提高性能

    E)        类的接口是否定义良好,如参数类型等,避免内部转换

    F)        是否采用内存或硬盘缓冲机制以提高效率

    G)        并发访问时的应对策略

    H)        I/O方面是否使用了合适的类或采用良好的方法以提高性能(如减少序列化,使用buffer类封装流等)

    I)          同步方法的使用是否得当,是否过度使用

    J)          递归方法中的叠代次数是否合适,应该保证在合理的栈空间范围内

    K)        如果调用了阻塞方法,是否考虑了保证性能的措施

    L)         避免过度优化,对性能要求高的代码是否使用profile工具,如Jprobe等

    4 资源泄漏处理方面检查项
       对于JAVA来说由于存在垃圾收集机制,所以内存泄漏不是太明显,但使用不当,仍然存在内存泄漏的问题。而对于其它的语言,如C++等在这方面就要严重关注了。当然数据库连接资源不释放的问题也是广大程序员最常见的,相信有很多的PM被这个问题折磨的死去活来。

    A)        分配的内存是否释放,尤其在错误处理路径上(对非JAVA类)

    B)         错误发生时是否所有的对象被释放,如数据库连接、Socket、文件等

    C)        是否同一个对象被释放多次(对非JAVA类)

    D)        代码是否保存准确的对象reference计数(对非JAVA类)

    5 线程安全方面检查项
    线程安全问题实际涉及两个方面,一个是性能,另一个是资源的一致性,我们需要在这两方面做个权衡,现在就是到了权衡利弊的时候了。

    A)        代码中所有的全局变量是否是线程安全的

    B)         需要被多个线程访问的对象是否线程安全,检查有无通过同步方法保护

    C)        同步对象上的锁是否按相同的顺序获得和释放以避免死锁,注意错误处理代码

    D)        是否存在可能的死锁或是竞争,当用到多个锁时,避免出现类似情况:线程A获得锁1,然后锁2,线程B获得锁2,然后锁1

    E)         在保证线程安全的同时,要注意避免过度使用同步,导致性能降低

    6 程序流程方面检查项
    A)        循环结束条件是否准确

    B)         是否避免了死循环的产生

    C)        对循环的处理是否合适,如循环变量,局部对象,循环次数等能够考虑到性能方面的影响

    7 数据库处理方面
    很多Code Review人员在面对代码中涉及到的数据库可移植性和提高数据库性能方面的冲突时表现的无所适从,凡事很难两全其美的啊。

    A)        数据库设计或SQL语句是否便于移植(注意和性能方面会存在冲突)

    B)        数据库资源是否正常关闭和释放

    C)        数据库访问模块是否正确封装,便于管理和提高性能

    D)        是否采用合适的事务隔离级别

    E)        是否采用存储过程以提高性能

    F)         是否采用PreparedStatement以提高性能

    8 通讯方面检查项
    A)        socket通讯是否存在长期阻塞问题

    B)        发送接收的数据流是否采用缓冲机制

    C)        socket超时处理,异常处理

    D)        数据传输的流量控制问题

    9 JAVA对象处理方面检查项
    这个检查项的基础是对JAVA对象有较深的理解,但现实是很多看过《Thinking in Java》的程序员,仍然在程序中无法区分传值和传引用,以及对象和reference的区别。这或许就是理论和实践难以结合的问题啊。正所谓知而不行,非真知也。

    A)        对象生命周期的处理,是否对象的reference已经失效,能够设置为null,并被回收

    B)        在对象的传值和传参方面有无问题,对象的clone方法使用是否过度

    C)        是否大量经常的创建临时对象

    D)        是否尽量使用局部对象(堆栈对象)

    E)         在只需要对象reference的地方是否创建了新的对象实例

    10 异常处理方面检查项
    JAVA中提供了方便的异常处理机制,但普遍存在的是异常被捕获,但并没有得到处理。我们可以打开一段代码,最常见的现象是进入某个方法后,一个大的try/catch将所有代码行括住,然后在catch中将异常打印到控制台,而且该异常是Exception对象。

    A)        每次当方法返回时是否正确处理了异常,如最简单的处理,记录日志到日志文件中

    B)         是否对数据的值和范围是否合法进行校验,包括采用断言(assertion)

    C)        在出错路径上是否所有的资源和内存都已经释放

    D)        所有抛出的异常都得到正确的处理,特别是对子方法抛出的异常,在整个调用栈中必须能够被捕捉并处理

    E)         当调用导致错误发生时,方法的调用者应该得到一个通知

    F)         不要忘了对错误处理部分的代码进行测试,很多代码在正常情况下执行良好,而一旦出错,整个系统就崩溃了

    11 方法(函数)方面检查项
    A)        方法的参数是否都做了校验

    B)         数组类结构是否做了边界校验

    C)        变量在使用前是否做了初始化

    D)        返回堆对象的reference,不要返回栈对象的reference

    E)         方法API是否被良好定义,即是否尽量面向接口编程,便于维护和重构

    12 安全方面检查项
    A)        对命令行执行的代码,需要详细检查命令行参数

    B)         WEB类程序检查是否对访问参数进行合法性验证

    C)        重要信息的保存是否选用合适的加密算法

    D)        通讯时考虑是否选用安全的通讯方式

    13 其他
    A)       日志是否正常输出和控制

    B)         配置信息如何获得,是否有硬编码

    三、总结
    通过在项目中实施Code Review将为我们带来多方面的好处,表现在提高代码质量,保证项目或产品的稳定性,开发经验的积累等,具体的实施当然也要看项目的实际情况,因为Code Review也是需要成本的,这方面属于Code Review过程的问题,将在其他文章中进行探讨。

    展开全文
  • Google: 如何code review

    千次阅读 2019-10-17 22:49:53
    导语:Google 前几天公开了一篇谷歌的工程实践文档,内容跟 code review 相关,里面包含了 Google 工程师如何进行 code review 的内容,以及 code review 指南。笔者将其转译成中文,以便大家参考学习。 原文地址:...

    导语:Google 前几天公开了一篇谷歌的工程实践文档,内容跟 code review 相关,里面包含了 Google 工程师如何进行 code review 的内容,以及 code review 指南。笔者将其转译成中文,以便大家参考学习。

    原文地址: https://google.github.io/eng-practices/review/reviewer

    本文的名词解释:

    • cr: code review

    • cl: change list,指这次改动

    • reviewer: cr的那个review人

    • nit: 全称nitpick,意思是鸡蛋里挑骨头

    • 作者: 也就是本次CL的开发者,原文中是以author来称开发者的。

     

    如果嫌文章太长,可以直接拖到文章最后看总结

     

    cr的标准

    cr(Code review)主要目的在于确保Google 的代码库代码质量越来越好。而所有相关的工具与流程皆是因应这个目的而生。为达到此目的,势必需要做出一连串的权衡与取舍。

     

    首先,开发人员必须能够在自己负责的任务上有所进展。如果你从来没有向代码库提交改进过的代码,那么代码库就永远不会改进。另外,如果一个reviewer使cr都很难进行的话,那么开发人员就不愿意在将来进行改进。

     

    另一方面,reviewer有责任确保每个change list(下称CL)的质量,保证其代码库的整体代码质量不会越来越差。这可能很棘手,因为通常会随着时间的推移,代码需要降级才能让代码运行起来,特别是当团队受到严重的时间限制时,大家觉得必须走捷径才能实现他们的目标。

     

    此外,reviewer对他们正在review的代码拥有所有权和责任。他们希望确保代码保持一致、可维护,以及下文的“在cr中可以得到什么”中提到的内容。

     

    因此,我们有以下规则,作为我们在cr中所期望的标准:

     

    一般来说,当CL存在的时候,reviewer应该赞成它,此时它肯定会提高正在工作的系统的整体代码质量,即使这个CL并不完美。这是所有cr中的首要原则。当然,这是有局限性的。例如,如果一个CL添加了一个reviewer不希望在其系统中使用的功能,那么reviewer当然可以拒绝,即使代码设计得很好。

     

    这里的一个关键点是,没有“完美”的代码,只有更好的代码。reviewer不应该要求作者在approve之前对一篇文章的每一小段进行润色。相反,reviewer应该权衡发展的需要和他们所建议的change的重要性。reviewer不应追求完美,而应追求持续改进。作为一个整体,提高系统的可维护性、可读性和可理解性的CL不应该因为它不是“完美的”而被延迟几天或几周。

     

    reviewer应该随时可以留下评论,表达一些更好的东西,但如果不是很重要,可以在评论前加上“nit:”这样的前缀,让作者知道这只是一个他们可以选择忽略的修饰点。

     

    指导

    cr有一个重要的功能,教开发人员一些关于语言、框架或一般软件设计原则的新知识。留下有助于开发人员学习新知识的评论是可以的。随着时间的推移,共享知识是提高系统代码健康度的一部分。你只需要记住,如果你的评论纯粹是教育性的,但对达到本文档中描述的标准并不重要,请在其前面加上“nit:”,或者以其他方式表明作者不必在本CL中解决它。

     

    原则

    现实和数据推翻了个人喜好。在代码风格方面,风格指南(style guide)是绝对的权威。任何不在style guide中的一些点(如空格等)都是个人偏好的问题。代码风格应该与现有的一致。如果项目没有以前的统一风格,那就接受作者的风格。

     

    软件设计的各个方面从来都不仅仅是一个纯粹的代码风格问题或者是个人喜好问题。它们是以基本原则为基础的,应当以这些原则为依据,而不仅仅是以个人意见为依据,有时几乎都没有选择的。如果作者能够证明(通过数据或基于原理的一些事实)他的方法是同样有效的,那么reviewer应该接受作者的偏好。否则,代码风格选择取决于软件设计的标准原则。

     

    如果没有其他规则适用,那么reviewer可以要求作者与当前代码库中的内容保持一致,只要这些代码不会恶化系统的整体代码健康状况。

     

    解决冲突

    在cr的任何冲突中,第一步应该始终是开发人员和reviewer根据本文和《CL Author’s Guide》,尝试达成共识。

     

    当达成共识变得特别困难时,reviewer和作者需要进行面对面会议,而不是仅仅试图通过cr的注释来解决冲突(不过,如果这样做,请确保将讨论结果记录在CL的评论中,以供将来的读者阅读)。

     

    如果这不能解决问题,最常见的解决方法就是升级。通常情况下,升级的途径是进行更广泛的团队讨论,让team leader参与进来,请求代码维护人员做出决定,或者请求技术经理提供帮助。不要因为作者和审稿人不能达成一致意见而让一个其他人袖手旁观。

     

    在cr中要看些什么

    注意:在考虑每一点时,一定要考虑cr的标准。

     

    设计

    在cr中重要的是看CL的总体设计。CL中不同代码段的交互是否有意义?此更改属于你的业务代码库还是属于引进来的其他代码库?它是否与系统的其他部分很好地集成?现在是添加此功能的合适时机吗?

     

    功能

    这个CL做了开发者想要的吗?开发者对这些代码的设计初衷用户有好处吗?“用户”通常既是最终用户(当他们受到更改的影响时)又是开发人员(他们将来必须“使用”此代码)。

     

    大多数情况下,我们希望开发人员在进行cr时能够对CL进行充分的测试,使其能够正常工作。但是,作为reviewer,仍然应该考虑边缘状况,寻找问题,尝试像用户一样思考,并确保仅通过阅读代码就不会看到错误。

     

    如果你愿意的话,你可以自己去验证CL。如果改动会直接带来的用户可见的影响,比如说ui改动,验证CL的变化是非常关键的。

     

    在阅读代码时,很难理解某些更改会对用户产生怎样的影响。对于这样的更改,如果不方便自己测试,则可以让开发人员演示该功能(demo)。

     

    另外,在cr期间考虑功能性特别重要的点是,cl中是否并发式编程,理论上可能导致死锁或竞争条件。这些类型的问题很难通过运行代码来发现,通常需要有人(开发人员和reviewer)仔细考虑,以确保不会引入问题(注意,这也是不使用平行式编程的一个很好的理由,在这种情况下,可能出现竞争条件或死锁,这会使代码检查或理解代码变得非常复杂)。

     

    复杂性

    一个CL是否复杂到超过预期的必须?针对任何层级的CL必须确认这几点:单行代码是否过于复杂?函数是否过于复杂?class是否过于复杂?“复杂”通常意味着该代码很难阅读,很有可能也意味着其他开发者在修改这段代码的时候引入其他bug。

     

    其中一种复杂性就是过度设计(Over-engineering)造成的,开发者会让那段代码过度通用化,超过了原本所需,或者还新增了系统目前不需要的功能。reviewer应特别注意一下过度设计。鼓励开发者解决他们知道现在需要解决的问题,而不是推测将来可能需要解决的问题。当那些将来出现的问题出现的时候才开始解决它们,因为那时候你可以更加清晰看见问题的原样子。

     

    Donald Knuth说过:过早的优化是万恶之源 (Premature optimization is the root of all evil)。

     

    测试

    请将单元测试、整合测试、端到端测试视为要求CL所做的适当变更。一般CL内除了生产环境的业务代码外,测试也应该要被加入其中。除非该CL是为了处理某个紧急事情而存在。

     

    另外,也要确保测试是正确、合理、有用的。测试并非来测试它们本身,一般也极少为了测试而测试(如测试一下测试代码有没有问题又走了测试流程),因此我们要保证测试是有效的。

     

    当代码真的有问题,测试是否会失败?如果被测试的程序发生改动时,测试是否会产生误报?每一个测试是否做出了简单而有用的断言?不同的测试方法之间的测试是否适当分开?

     

    请记住,测试代码也是必须维护的代码,不要因为它们不在主要关注的范围内。

     

    命名

    开发者是否为了每一个东西都挑了一个适当的名字?一个好的命名意味着,通过名字就足以完整表达该东西的作用是什么或者要做什么。但是同时名字也不要长得难以阅读。

     

    推荐参考文章:Clean code 101 — Meaningful names and functions

     

    注释

    开发者是否用可理解的英文留下清晰的注释? 这些注释是否真的必要?

     

    通常注释是解析这段代码为什么存在的时候是相当有用的,而不应该去解释某段代码正在做什么。如果代码本身不能解释清楚的话,意味着它更加需要简化了。当然也有例外,比如解释正规的表达式或者复杂的算法正在做什么的时候,注释解释这段代码正在做什么就相当有用。但对于大部分注释来说是用来说明那些不包含在程序本身但资讯,比如说为什么要这样子做的理由。

     

    查看该CL之前的注释也很有帮助,或许有一个todo项目现在可以一处、一个评论建议为什么不要进行这种更改等等。

     

    要注意的是,注释与class、module、function的文件不同。后三者要能够表达一段代码的目的、如何使用它、使用时行为。

     

    风格

    Google 对于主要语言都有提供风格指南(style guide),甚至包括大多数冷门语言,因此要确保CL遵循适当的指南上的说明。

     

    如果你想改进CL中某些不包含在风格指南中的要点时,请在评论前加上Nit: ,让开发人员知道这是你认为可以改善代码的小问题且并非强制性的。但记住,不要仅根据个人风格偏好阻止提交CL。

     

    开发者不应该在 CL内同时包含主要风格的改动与其他代码的修改,这样会导致难以看出CL到底做出什么改动。同时也会让合并和回滚更为复杂,并产生其他问题。例如,如果作者想要重新格式化代码的话,让他们将新格式在一个新CL里面重构。

     

    文档

    如果CL更改了构建、测试、交互、发布的时候,请检查是否也同时更新相关文档, 包括README,g3doc 页面和其他生成(generated) 的参考文件。如果CL 删除或弃用 了一些代码,请考虑是否应该删除对应文档,如果缺少文档时请询问。

     

    每一行代码

    仔细review分配给你的每一行代码。有些东西,比如资料文件(data files)、生成的代码(generated code)、大型数据结构(large data structures),你可以稍微扫过。千万不要在扫过开发者写的 class、函数、代码区块 时,去假设它内部是没问题的。很显然的某些代码需要比其他代码更仔细的review。这是必须由你做出的判断,但至少你应该确定你理解所有代码在做些什么。

     

    如果阅读代码过于复杂并且减慢review速度时,那么你再继续review前,要让开发者知道这件事,并等待他们为这段代码做出解释、说清楚。在Google 我们聘请许多优秀的软件工程师,而你也是其中的一员。如果连你也无法理解的话,很可能其他人也不会。因此,你要求开发者去说清楚这段代码时,同时也在帮助未来的开发人员理解这些代码。

     

    如果你能够理解,但觉得没有资格进行某部分的审核,请确保 reviewer中有一个适合(合格) 的人来review该部分。尤其是针对安全性、并发性、可访问性、国际化等复杂问题时。

     

    上下文

    在充足的上下文下查看CL通常很有帮助。一般来说,cr工具只会显示修改部分周围的几行代码而已。但有时你必须查看整个文件以确保改动是否合理。比方说,你可能只看到添加4行新代码,但实际上查看整个文件时会发现这4行是被加在有50行的代码里,此时需要将它拆解为更小的函数。

     

    以整个系统的角度出发来思考CL也是很有用的。该CL是否改善整体系统的代码质量,亦或是会让整个系统更加复杂?是否缺少测试?千万不要接受会降低整体系统的代码质量的CL。因为大多数系统是由于许多小改动的积累而变得复杂,因此阻止新的改动引入复杂性(尽管很小)也非常重要。

     

    优点

    当你在CL里看到优点时记得告诉开发者,尤其是当他们用很棒的方式来解决你的评论时。cr通常只关注存在的错误,但它们也应该同时应该为良好实践提供鼓励与欣赏。这点在指导开发者时尤其重要:与其告诉他们做错什么,还不如告诉他们做对了什么。

     

    个人认为并非不要指出错误,而是多以鼓励来代替指出错误,让其他开发者更有动力想将事情做好。其实透过简单的一句话让对方知道哪里做得很好,未来他们会将继续保持下去,并为其他开发者带来的正面影响。

    标明每个commit 修改什么,帮助reviewer快速了解情况

     

    此时就不要吝啬你的称赞了!

     

    总结

    在cr时,请务必确保:

    • 代码经过完善的设计

    • 功能性对于使用者们是好的

    • 对于任何UI改动要合理且好看

    • 任何并行编程的实现是安全

    • 代码不应该复杂超过原本所必须的

    • 开发者不该实现一个现在不用而未来可能需要的功能

    • 代码有适当的单元测试

    • 测试经过完善的设计

    • 开发者对于每样东西有使用清晰、明了的命名

    • 注释要清楚且有用,并只用来解释why而非what

    • 代码有适当的写下文件(一般在g3doc)

    • 代码风格符合style guide

    • 确保你查看被要求review的每一行代码、确认上下文、确保你正在改善代码质量,并赞扬开发人员所做的好事与优点吧!

       

    如何浏览CL

    现在你已经知道review时要看些什么,但怎样才是review分散在多个文件中的改动最有效的方法?

     

    1. 改动是否合理?他是否有好的描述(description)

    2. 优先看CL 最重要的改动。它整体是否有完善的设计?

    3. 用合理的顺序看CL 剩余的改动

     

    步骤1: 用宏观的角度来看待改动,查看CL描述以及它做什么

    该改动是否有意义、合理?如果在第一时间认为不应该发生这种变化,请立即说明为什么不该这样做的原因。当拒绝类似这样的更改时,向开发人员提供建议告诉他们应该怎么做什么也是一个好主意。

     

    例如,你可以说:“看起来你已经完成一些不错的事情,谢谢!但我们实际上正朝着删除你正在修改的FooWidget系统的方向前进,所以现阶段我们不想对它进行任何新的修改。不如重构我们的新BarWidget class如何?”

     

    需要注意的是,reviewer在委婉拒绝该CL的同时也要提供替代方案,而且仍然保持礼貌。这种礼貌是很重要,因为我们希望表明即使不同意也会相互保持尊重。

     

    如果你有几个CL里包含你不想这样做的改动时,你应该重新考虑开发团队的开发过程,或发布开发流程给外部贡献者知道,以便在任何CL被撰写前有更多的沟通。最好是在他们开始投入前说“不”,避免那些已经投入心血的工作现在必须被抛弃或彻底重写。

     

    提供替代方案让对方知道该怎么做,而非让其自行独自摸索。

    指出问题,告知替代方案或指点方向

     

    步骤2: 检查CL主要的部分

    找到CL最核心的部分的那些文件。通常CL内会有文件包含大量的逻辑改动,而它正是CL的主要部分。因此我们要首先查看这些主要部分。这有助于为CL的其他较小部分提供适当上下文,而且这样通常可以提高review速度。如果CL太大导致于无法确定哪里是主要部分时,请向开发者询问首先应当查看的内容,或者要求他们将CL拆分为多个CL。

     

    如果在主要部分发现存在一些主要的设计问题时,即使没有时间立即查看CL的其余部分,也应立即留下评论告知此问题。因为事实上,因为该设计问题足够严重的话,继续review其他部分的代码可能只是浪费时间,因为其他正在审查的其他代码可能都将无关紧或消失。

     

    立刻发送关于主要设计的评论非常重要有两个主要原因:

     

    • 通常开发者在送出CL后,在等待review过程中便已经开始着手基于该CL的新工作。此时如果正在review的CL存在重大设计问题的话,开发者将不得不重新写所有基于它的后续所有CL。因此你要在他们有问题的设计上投入之前阻止他们。

    • 主要设计变更通常需要较长时间才能完成,但每个开发人员几乎都有自己deadline。为了赶上deadline并保证代码质量,开发者需要尽快开始或重做CL 任何的主要设计变更。

     

    步骤3: 用合理的顺序看CL 其余的改动

    一旦确认整个CL没有重大的设计问题时,试着找出一个有逻辑顺序来review剩余档案,并确保不会错过其中任何一个。通常在浏览主要部分后,最简单的方式是按照cr工具提供的顺序来浏览每个文件。有时在阅读主要代码前先阅读测试也是非常有帮助的,如此一来你就可以了解应该做、看些什么。

     

    review的速度

    为什么review速度要快

    在Google我们优化开发团队共同生产产品的速度,而不是优化个人开发的速度。个人的开发速度很重要,但它不如整个团队的开发速度重要。当cr很慢时,会发生以下几件事:

    • 团队整体的速度下降。review慢的话,对于团队其他人来说重要的新功能与缺陷修复将会被延迟数天、数周甚至数月,只因为它们正在或者等待review。

    • 开发人员开始抗议cr。若reviewer每隔几天才回应一次,但每次都要求对CL进行重大更改的话,那么开发人员可能会感到非常沮丧与觉得困难,而这通常会演变成抱怨。如果reviewer请求相同的实质性更改(且确实可以改善代码质量状况),但在每次开发人员提交新的改动时都能快速反应的话,这些抱怨往往会消失。大多数关于cr的抱怨,往往可以通过加快流程来解决。

    • 代码质量会受到影响。当review很慢时,会造成开发者提交不完全尽如人意的CL 的压力越来越大。评论的越慢也会阻止他人进行代码清理、重构、甚至是对现有CL 的进一步改进。

     

    cr应该要多快?

    如果你并没有处于需要专注工作的时候,那么你应该在CL被提交后尽快进行review。review回复最长的极限是一个工作日。若遵循以上指南,意味着一般的CL应该在一天内得到多轮review(如果必要的话)。

     

    速度vs中断

    但有时候个人的速度优先度会胜过团队速度。如果你处于需要专注工作的时候(比方说写代码),不要打断自己去做cr。

     

    研究证实,若开发者在被打断后会需要很长时间才能恢复到原本顺畅的开发流程。因此,开发的时候,打断自己实际上会比让另一位开发者等待review来得更加昂贵。

     

    取而代之的是,我们可以在投入到处理他人给的review评论之前,找个适当的时机点来进行cr。这有可能是当你的当前开发任务完成后、午餐、刚从会议脱身或从微型厨房回来等等。

     

    快速回应

    当我们谈论cr的速度时,我们关注的是回应时间,而非CL需要多长时间才能完成并提交。在理想情况下,整个过程应该是快速的。

     

    总的来说个人回应评论的速度,比起让整个cr过程快速结束来得更为重要。即使有时需要很长时间才能完成整个流程,但若在整个过程中能快速获得来自reviewer的回应,这将会大大减轻开发人员对于缓慢的cr过程的挫败感。

     

    如果真的忙到难以抽身而无法对CL进行全面review时,你依然可以快速的回应让开发者知道你什么时候会开始审核、建议其他能够更快回复reviewer,又或者提供一些初步的广泛评论。(注意:这并不意味着你应该中断开发去回复——请找到适当的中断时间点去做)。    

     

    很重要的是,reviewer员要花足够的时间来进行review,确保他们给出的LGTM,意味着“此代码符合我们的标准”。

     

    尽管如此,理想的个人的回应速度还是越快越好。

     

    跨时区review

    在面对时区不同的问题时,尽量在他们还在办公室时回复作者。如果他们已经回家了,那么请确保在他们第二天回到办公室前完成。

     

    LGTM 评论

    为加快速度,在某些情况下reviewer可以给予LGTM或Approval,即便CL上仍有未解决的评论。类似情况会发生在:

    • reviewer确信开发人员会适当地处理所有剩余的评论

    • 剩余的评论是微不足道的或它们不需由开发者来处理

    • reviewer必须清楚指明他们是指上面哪种情况

     

    LGTM 评论对双方处于不同的时区时尤其值得考虑,否则开发人员会等待一整天才得到“LGTM,Approval”。

     

    大型改动

    如果有人要求reivew时,但由于改动过于庞大导致你难以确定何时才有时间review它时,你通常该做的是要求开发人员将CL拆解成多个较小的CL,而不是一次review巨大的CL。这种事是可能发生的,而且对于reviewer非常有帮助,即便它需要开发人员付出额外人力来处理。

     

    如果CL无法分解为较小的CL,且你没有足够时间快速查看整个CL内容时,那么至少对它的整体设计写些评论,并发送回开发人员以便进行改进。身为reviewer,你的目标之一是在不牺牲代码质量的状况下,避免阻挡开发人员进度,或让他们能够快速采取其他更进一步的动作。

     

    cr的能力会随着时间进步

    如果你遵循这些准则,并且对于cr非常严格的话,后面你会发现整个cr流程会越来越快。因为开发者学到什么是质量好的代码,并于开头就提交一个很棒的CL,而这正恰好只需要越来越少的时间。而reviewer则学会快速回应,而非在过程中加入不必要的延期。

     

    但不要为提高想象中的速度,而对cr标准和代码质量做出妥协,毕竟从长远来看它实际上并不会让任何事情发生得更快。

     

    紧急状况

    在某些紧急情况下,CL会希望放宽标准以求迅速地通过整个cr过程。但请看什么是紧急情况来知道哪些情况实际上属于紧急情况,而哪些不符合。

     

    如何写review评论

    如何面对被推迟处理的评论

    有时开发人员会推迟处理cr产生的评论。要么他们不同意你的建议,要么他们会抱怨你太严格了。

     

    谁是对的

    当开发人员不同意你的建议时,请先花点时间考虑一下他们是否是正确的。因为通常他们比你更了解代码,所以他们可能真的比起你来说对代码的某些层面具有更好的洞察力。他们的论点有意义吗?从代码质量的角度来看它是否合理?如果是的话,让他们知道他们是对的,然后让问题沉下去。

     

    但开发人员也并不总是对的。在这种情况下,reviewer应该更进一步解释为什么认为自己的建议是正确的。一个好的解释通常展示了“对开发人员的回覆的理解”以及有关“为什么被要求做出改动”等信息。尤其是当reviewer认为给出的建议会改善代码质量时,便应该继续宣扬自己的论点。只要他们相信所需的额外的工作量最终会改善整体代码质量。提高整体代码质量这件事,往往是经由每个微小的改动来发生。有时往往需要几番解释一个建议才能让对方真正理解你的用意。切记,始终保持礼貌,让开发人员知道你有听到他们在说什么,只是你不同意该论点而已。

     

    让开发者沮丧

    reviewer有时会认为若自己坚持改进的话,会让开发人员觉得沮丧不安。的确开发人员有时会感到很沮丧,但这通常是十分短暂的,甚至后来他们非常感谢你帮助他们提高代码质量。一般来说,只要你在评论中表现得很有礼貌,开发人员实际上根本不会感到沮丧,这些担忧都仅存在于reviewer心中而已。沮丧很多时候是对于cr评论的写作方式有关,而并非来自reviewer对于代码质量的坚持。

     

    晚点再来整理干净

    一个常见的推迟原因是开发人员希望完成任务(这可以理解)。他们不想通过另一轮cr来批准这个CL。此时他们通常会说在以后的CL进行整理,所以你现在应该要给LGTM。当然部分开发人员非常擅长这一点,并且立即送出一个修复问题的后续CL (follow-up CL),但根据过往经验,这种后续“清理行为”非常少见。除非开发者在CL获得批准之后立刻进行清理动作,否则这事根本不可能发生。这不是因为开发人员不负责任,而是因为他们可能有很多其他工作要完成,于是清理工作便会在成堆的工作中被遗忘。因此,通常最好坚持开发人员在代码在合并后清理它们。因为让人们「稍后清理」是致使代码库质量状况下降最常见的状况。

     

    如果CL引入了新的复杂性的话,除非是紧急情况,否则必须在提交之前将其处理掉。如果CL导致暴露周围的问题且现在无法解决它们的话,开发人员应该将缺陷记录下来并分配给自己,避免后续被遗忘。又或者他们可以选择在代码中留下TODO的注释并链接到刚记录下的缺陷。

     

    关于review严格性的常见抱怨

    如果你先前以相当宽松的标准并转趋严格的进行cr的话,一些开发人员会开始大声地抱怨。一般来说,提高review的速度会让这些抱怨逐渐消失。这些抱怨可能需要数个月才会消失,但最终开发人员会看到严格的review所带来的价值,因为严格的review帮助他们产生的优秀代码。而且一旦发生某些事情时,最大声的抗议者甚至可能会成为你最坚定的支持者,因为他们会看到变得review变严格后所带来的价值。

     

    解决冲突

    如果你遵循前面所有操作,但仍然遇到无法解决的双方之间的冲突时,请参考前面的cr的标准以获取有助于解决冲突的准则和原则。

     

    译者总结

    cr的标准

    • reviewer 有责任保证CL的质量,作为reivew的代码的owner

    • reviewer不应追求完美,而应追求持续改进

    • cr具有指导意义

    • 代码风格应该与现有的一致。如果项目没有统一风格,那就接受作者的风格

    • 解决冲突难以达成共识时,需要面对面或者拉起更大的团队讨论,带上leader

     

    在cr中要看些什么

    • CL的总体设计

    • 功能验证,功能性对于使用者们是好的,对于任何UI改动要合理且好看

    • 是不是很复杂,有没有过度设计

    • 代码有适当的单元测试

    • 测试经过完善的设计

    • 规范命名,看见名字就知道是什么

    • 合适的注释,注释应该是why而不是what

    • 代码风格遵循style guide,如果需要改代码风格应该在另一个CL解决

    • CL更改了构建、测试、交互、发布的时候,也要更新文档

    • 仔细review每一行代码(除了资源文件、生成的代码、大型数据结构)。如果比较复杂得让开发者解释

    • 安全性、并发性、可访问性、国际化等复杂问题时需要更合适的人来review

    • 以整个系统的角度出发来思考CL

    • review的时候,与其告诉开发者做错什么,还不如告诉他们做对了什么

     

    如何浏览CL

    • 用宏观的角度来看待改动,查看CL描述以及它做什么

    • 检查CL主要的部分

    • 用合理的顺序看CL 其余的改动

     

    review的速度

    • review速度慢会导致团队整体的速度下降、开发人员开始抗议cr、代码质量会受到影响

    • 如果你处于需要专注工作的时候(比方说写代码),不要打断自己去做cr

    • 个人回应评论的速度,比起让整个cr过程快速结束来得更为重要

    • 在面对时区不同的问题时,尽量在他们还在办公室时回复作者

    • 为加快速度,在某些情况下reviewer可以给予LGTM或Approval,即便CL上仍有未解决的评论

    • 由于改动过大导致难以review时,通常该做的是要求开发人员将CL拆解成多个较小的CL

    • cr的速度应该要越来越快,但不要为提高想象中的速度,而对cr标准和代码质量做出妥协

     

    如何写review评论

    • 当开发人员不同意你的建议时,思考一下谁是正确的,并解释清楚,保持礼貌

    • 如果review坚持己见,会让开发者沮丧。沮丧很多时候是对于cr评论的写作方式有关,并非来自reviewer对于代码质量的坚持

    • 如果CL引入了新的问题的话,除非是紧急情况,否则必须在提交之前将其处理掉

    • 如果现在无法解决review的评论的问题的话,TODO的注释并链接到刚记录下的缺陷

     

    review太严格被抱怨该怎么办

    • 提高review的速度会让这些抱怨逐渐消失。这些抱怨可能需要数个月才消失,但最终开发人员会看到严格的review所带来的价值,因为严格的review帮助他们产生的优秀代码

    展开全文
  • 谈谈我们公司如何Code Review

    千次阅读 2018-12-13 21:18:01
    研发中心团队越来越庞大了,开发人员越来越多了。和他们聊天过程中,发现开发人员对代码技能的提升很迷茫,诉求越来越浓厚。只不过一个接一个的项目交付没有给...但是最被认可的还是Code Review活动。  那么 Code...
  • gitlab 管理代码code review

    万次阅读 2017-02-17 21:20:38
    场景:你拉了一个远程项目parent,这个项目是老大的,老大也把你加进来了,然而你要提交代码的时候却不能提到parent中,因为你要吧代码给老大code review,如果你使用的是gerrit + jenkins最好不过了,就不用看...
  • 如何有效的Code Review

    万次阅读 2012-07-17 13:31:01
    如何有效的Code Review 什么是Code Review? Code Review代码评审是指在软件开发过程中,通过对源代码进行系统性检查的过程。通常的目的是查找各种缺陷,包括代码缺陷、功能实现问题、编码合理性、性能优化...
  • 论文必须知道什么叫review

    万次阅读 2016-07-06 16:56:33
    老师听到老师学长说到文献综述,也就是review,突然有点莫名起来,数字资源资料库里面我该如何搜索呢?好像很多人的本科论文都是瞎混混过来的,我也是,有看过期刊和一些论文,却不明白到底如何来找文献综述。 ...
  • [读后感]从Code Review 谈如何技术

    千次阅读 2014-11-16 05:33:31
    [读后感]从Code Review 谈如何技术
  • 我们是怎么Code Review

    千次阅读 2019-09-28 16:23:04
    前几天看了《Code Review 程序员的寄望与哀伤》,想到我们团队开展Code Review也有2年了,结果还算比较满意,有些经验应该可以和大家一起分享、探讨。我们为什么要推行Code Review呢?我们当时面临着代码混乱、Bug频...
  • 听我的!美国科技公司这样Code Review.pdf
  • 如何用github/gitlab代码review

    万次阅读 2015-12-07 16:11:40
    本文背景:由于ReviewBoard非常水,diff稍微大一点就会提交失败。那么如何做review呢?不妨利用github/gitlab自带的在线Diff展示功能。操作过程...
  • 如何用 gitlab code review

    万次阅读 2015-05-08 21:49:17
    workspace / working directory:工作区 就是你在电脑里能看到的目录 index / stage:暂存区 更改通过git add到了这里 repository:版本库 git commit更改到这里 (之前三个概念可参照工作区、暂存区 和 版本库
  • 使用Phabricator为Code Review工具

    千次阅读 2017-11-12 21:48:16
    目录 0x10 概述 0x20 我的应用环境 0x30 路线图 0x40 安装 0x41 LNMP环境的安装 0x42 Phabricator源码下载及运行 0x50 配置 0x51 解决基本的配置问题 ...0x60 使用Phabricator进行Code Review 0x61
  • 开发人员需代码审查(Code Review)的5个原因.pdf
  • CodeReview工具

    2020-12-04 21:16:15
    包含jupiter和reviewclipse两款eclipse插件,code review使用。有效的code review能有效改善bug多发,代码质量低下等问题
  • 大家是怎么Code Review的?

    千次阅读 2013-09-11 17:58:41
    先说说我们公司现在的做法,一个团队被人为地分为两个阵营:Senior Developers和Junior Developers,比例差不多是1:1,Senior Developers就担负着对Junior Developers的代码进行Review的职责,每天Review一次,对有...
  • 刚才读了《软件质量保证的最佳实践之一:Code review和Case review 》,有一感,就是说结合可以促进质量。我们常提倡默默工作,埋头苦干,在这个大协作的国际分工里头,好象行不通了。了要说,说了要,再...
  • 如何用 Gitlab 团队内的 Code Review

    千次阅读 2018-03-28 21:00:01
    基于分支的代码 Review新建 Issue (无论是 bug 还是 feature), 描述背景或问题,本地创建分支 issue#123 (123是 issue 的 ID), 围绕关联 issue 进行 program -> commit -> push,新建 Merge Request ...

空空如也

空空如也

1 2 3 4 5 ... 20
收藏数 76,947
精华内容 30,778
关键字:

工作review怎么做