在去年发生了一些严重的质量问题之后,我公司最近引入了代码审查。代码审查过程很快就引入了,没有任何指导方针或任何形式的清单。

我们还被选为“技术主管”。这意味着我们对代码质量负责,但我们无权执行流程中的更改,重新分配开发人员或保留项目。

从技术上讲,我们可以拒绝合并,将其归还发展。实际上,这几乎总是以我们的老板要求准时交货为前提。

我们的经理是MBA,主要关心制定近期项目的时间表。在尝试过程中,他几乎不知道从业务角度来看我们的软件的功能,并且即使没有开发人员的解释,也难以理解最基本的客户需求。

当前开发已完成在SVN的开发分支中,在开发人员认为他已经准备好之后,他将票务系统中的票证重新分配给我们的经理。经理然后将其分配给我们。

代码审查导致我们团队内部出现一些紧张关系。尤其是一些年长的成员对更改提出了质疑(即“我们总是这样做”或“为什么该方法应有一个明智的名称,我知道它的作用?”。)几周后,我的同事开始放任自流,以免给同事造成麻烦(她告诉我自己,在客户提交错误报告后,她知道该错误,但担心开发人员会

另一方面,我现在因指出已提交代码的问题而臭名昭著。

我不知道不要以为我的标准太高。

目前,我的清单是:


该代码将编译。
至少有一种方法可以运行该代码。
该代码将在大多数普通情况下运行。
该代码将在大多数情况下运行。
如果插入的数据无效,则代码将引发合理的异常。

但是我完全接受提供反馈的方式的责任。我已经在提出可行的观点,解释了为什么应该更改某些内容,有时甚至只是问为什么要以特定的方式实现某些内容。当我认为不好的时候,我指出我会以另一种方式开发它。

我所缺乏的是找到能够指出“好”的东西的能力。我读到有人应该把坏消息夹在好消息中。

但是我很难找到一件好事。 “嘿,这次您实际上投入了您所做的所有事情”比高尚或有用的方法更具有尊严。

示例代码回顾br />我对在Library \ ACME \ ExtractOrderMail类中所做的更改有一些疑问。此刻,对“ GetMails”的第二次调用将引发异常,因为您将文件添加到其中,但在删除它们后再也不会删除它们。我知道该函数每次运行仅被调用一次,但是将来可能会发生变化。
您可以将其设置为实例变量,然后我们就可以并行处理多个对象。 > ...(某些其他点无效)

次要点:


为什么“ GetErrorMailBody”将异常作为参数?我错过了什么?您不会抛出异常,只需将其传递并调用“ ToString”即可。为什么会这样?
SaveAndSend不是方法的好名字。如果邮件处理出错,则此方法发送错误邮件。您可以将其重命名为“ SendErrorMail”或类似名称吗?
请不要只注释旧代码,请将其彻底删除。我们还在Subversion中使用它。



评论

请不要拿出一个$ h!t的三明治来实现好与坏的神话般的平衡。如果他们做得很好,请告诉他们,如果他们做了需要纠正的事情,请告知他们。好的和坏的混合会稀释消息。如果他们得到的负面反馈多于正面反馈,也许他们会意识到他们需要改变。您的三明治方法为每个否定比率提供2:1的比率,因此最终得到的净收益就是您要发送的消息。

停止使用第二人称。代码是主题,而不是编码器。例如,写:SaveAndSend应该重命名以更好地适应其行为,例如SendErrorMail。现在,即使您已经“四处散播”所有“请问”,您看起来真的像是在向同事下达命令。我不会接受审稿人的。我更喜欢直接说“必须完成”的人,而不是要我(甚至礼貌地)做点什么。

“我读过,应该将坏消息夹在好消息中”。您需要确保已经有一个清晰的全球性了解,这不是代码审查。它们不像员工绩效评论或电影评论那样好坏。它们更像是质量检查流程的一部分。您不会期望您的测试人员创建票证说“此功能很棒,并且可以按我期望的那样工作!”,而且您也不应该在代码审查中期望它。

我认为您的第一步应该是创建一套基本的代码标准/准则,允许其他成员提供反馈,并基本上从所有人那里获得“买入” //同意准则是“在合理范围内”的协议。然后,他们都知道他们同意被拘留。上一个效果很好。我曾在的公司。

不要使用“但将来可能会改变”这一短语。仅针对现在需要的内容进行编码。不要为将来可能发生或可能不会发生的变化增加复杂性。如果您肯定知道它会改变,那会有所不同,但这并不是偶然的机会,它可能会改变。

#1 楼


如何在代码审查中找到积极的东西?
在去年发生了一些严重的质量问题之后,我公司最近推出了代码审查。

太好了,有机会为您的公司创造价值。

最初的几周后,我的同事开始放任自流,以免对同事造成麻烦(她告诉我自己,
客户提交了一个错误报告,她知道该错误,但
担心开发人员会因为指出错误而对她感到生气)。

您的同事应该如果她不能告诉开发人员代码有什么问题,请不要进行代码审查。找到问题并在问题影响到客户之前解决问题是您的工作。
同样,恐吓同事的开发人员也要被解雇。经过代码审查后,我感到很害怕-我告诉我的老板,它已得到处理。另外,我喜欢我的工作,因此我保持正面和负面的反馈。作为审阅者,这是我的责任,而不是其他任何人。 >好吧,很不幸,您说自己很机智。如果您有更多需要的地方,可以找到更多值得称赞的东西。
批判代码,而不是作者
您举一个例子:在

中避免使用单词“ you”和“ your”,而改为“ the”。

我错过了什么吗? [...]为什么?

不要在您的评论中添加夸张的夸张。也不要开玩笑。我听过一个规则,“如果让你觉得好说,不要说,那就不好了。”
也许你是在自欺欺人,以别人为代价。坚持事实。
通过提供积极的反馈来提高标准
提高标准,以表彰您的开发人员达到更高的标准。这就是问题所在,

如何在代码审查中找到积极的东西?

是一个很好的例子,值得解决。
您可以指出代码在哪里符合更高级别的编码实践的理想。
寻找他们遵循最佳实践,并不断提高标准。在每个人都期望获得更简单的理想之后,您将不应该再赞扬这些理想,而是寻求更好的编码实践来称赞。面向功能的或功能性的编程功能,您可以大声疾呼,并祝贺作者在适当的地方使用它们。这些问题通常属于样式指南:

是否符合内部语言样式指南标准?
它是否符合该语言最权威的样式指南(可能比该指南更严格内部-因此仍然符合内部风格)?

通用最佳实践
您可以在各种范式下找到赞美通用编码原则的要点。例如,他们是否具有良好的单元测试?单元测试是否涵盖了大多数代码?代码覆盖率,以及对API和语义公共功能的完整测试。
验收测试和冒烟测试可以测试端到端功能,包括为单元测试模拟的功能。要点,所以代码是DRY(不要重复自己),没有魔术字符串或数字。
变量的命名是如此出色,以至于注释在很大程度上是多余的。
清理,客观改进(不需权衡)和适当的重构。可以减少代码行数和技术负担,而又不会使代码与原始作者完全陌生。

函数式编程
如果语言是功能性语言,或支持功能性范式,请寻找以下理想:

使用闭包和部分函数避免全局和全局状态
具有可读性,正确性,和描述性名称
单个出口点,最大程度地减少了参数数

面向对象编程(OOP)
如果语言支持OOP,则可以赞扬这些功能的适当用法:

封装-提供一个清晰定义的小型公共接口,并隐藏细节。
继承-可能通过mixins适当地重用了代码。
多态性-定义了接口,也许是抽象的基础类,为支持参数多态性而编写的函数。

在OOP下,还有SOLID原则(可能对OOP功能有一些冗余): / owner
打开/关闭-不修改已建立对象的接口
Liskov替代ion-子类可以代替父级实例
接口隔离-由组合提供的接口,也许是mixins
依赖性反转-定义的接口-多态...

Unix编程原理:
Unix的原则是模块化,清晰度,组成,分离,简单,简约,透明,健壮,表示,最小惊喜,沉默,修复,经济,生成,优化,多样性和可扩展性。
这些原则可以在许多范式中应用。
您的条件
这些都太琐碎了-如果对此赞誉有加,我会觉得很屈从:

代码将编译。 />至少有一种方法可以使用该代码。
该代码可以在大多数普通情况下使用。

另一方面,考虑到您的实际情况,这些都是相当受好评的正与之打交道,我会毫不犹豫地称赞开发人员所做的事情:

该代码将适用于大多数情况。
如果插入的数据无效,则代码将引发合理的异常。

写下通过代码审查的规则?
从理论上讲,尽管我通常不会因为错误的命名而拒绝代码,但我看到命名是如此糟糕,以至于我会拒绝带有修复指令的代码。您必须能够出于任何原因拒绝代码。
我能想到的唯一拒绝代码的规则是,没有什么太可怕了,我无法将其拒之门外。我很乐意将一个不好的名字拒之门外-但您不能将其作为规则。如果语言支持它们,那么所有这些。

评论


我什至会争辩说,其中许多可以成为代码审查反馈模板的标题。这样就可以在多个标题下进行评论,例如“出色的工作”,而无需任何实际的额外费用。它还为同事们提供了如何使自己的代码更好的好主意。

–斯蒂芬
16-10-17在23:18

在列出许多良好做法的同时,您可能在回答错误的问题-因为这确实是一个xy问题。而且很难找到允许这些反馈的审查系统。重要的事情隐藏在无用的噪音中。有时,问题的答案只是“不要这样做-这是错误的方式。您的问题出在其他地方,应该适当解决。”如果人们开始专注于发现美好的事物,那么代码审查就成了浪费时间。您可以在午饭时告诉您的同事,他的实施有多好,他可能会很感激。

– Eiko
16-10-18在8:11

@亚伦:同意你的做法。这里有很多答案说“不要加糖”,但我知道这并不是要取悦所有人。人们在做好事时会更倾向于采用正确的方法,而不是被告知自己错了。他们在这里的关键是要机智但要保持一致。从OP的描述来看,他所在的团队并不完善,甚至老成员也习惯了。他们会更乐于接受温和的态度。

–Hoàng Long
16-10-21在1:43

@HoàngLong并非每个“老朋友”都一定会“更容易接受”。总有某个地方不讲理的人。例如,我曾经与一个坚持将他的Perl最佳实践“移植”到Python和Subversion's移植到Git的家伙一起工作,并且每次调用它时都会有某种抱怨,不管它是如何,即使推理进行了解释。由于当时我的责任落在了我的腿上(我是使用Python和Git的唯一经验者),所以我认为有些人可能会感到受到威胁(?)并做出相应的反应...

– code_dredd
17年2月11日在19:16

#2 楼

除非有一个可靠的简洁示例,并且与所关注的问题直接相关,否则请不要去挑选一些好的东西。对自己的能力不安全并且正在以不成熟的方式应对工作挑战的人。他们的工作也可能很糟糕-优秀的开发人员应该始终愿意反省,接受建设性的批评,并乐于改变自己的方式。您。不管您是否认为自己很合理,都需要对这样的人格外小心,以使事情顺利进行。我发现与这些人打交道的最好方法是,对自己的用语表达要非常小心。

确保您在责怪代码而不是作者。专注于手头的一个问题,而不是您的代码库的poo-mountain,他们可能在创建过程中发挥了重要作用,并被视为进一步的人身攻击。首先选择战斗,将注意力集中在向用户显示的关键问题上,这样就不会对导致他们拒绝您所说的一切的人发起批评。如果您要亲自与他们交谈,这很重要,请清楚您在说什么,并确保您没有与他们交谈或忽视他们的技术能力。他们很可能会立即采取防御措施,因此您需要解决他们的担忧而不是确认他们。您需要对这次谈话保持意识,不要太明显,以使他们下意识地认为您站在他们一边,并希望他们接受他们需要做出引起注意的更改。如果这不起作用,您可以开始变得更具攻击性。如果该产品可在会议室中运行,则在代码检查期间将其放在投影机上,并第一手显示该错误,如果经理在现场,则该功能更好,这样人们就不能退缩。这不是要让他们感到羞耻,而是要迫使他们接受这个问题,对用户来说是真实的,并且需要解决它,而不是仅仅困扰于他们的代码。您无所事事,厌倦了像幼儿园学生那样对待人,并且管理层完全没有意识到问题,这或者反映了您作为代码审查员的表现不佳,或者您关心公司的健康状况, /或产品,您需要开始与老板讨论他们的行为。尽可能具体和无个性-提出一个业务案例,证明生产力和质量正在受到损害。

评论


我发现作为审阅者和受审者有用的另一种策略是归因于第三方需要涵盖一些极端情况。我对那些担任管理职务的人表示歉意,但说了些类似的话,“我们需要考虑到这种极端情况,因为管理确实一直在拖尾,所以我们要确保这几乎是防弹的。给他们一种轻松的感觉。”听起来,无论如何,管理层都不介意成为OP的“坏人”。

– Greg Burghardt
16-10-17在21:06

@GregBurghardt嘿,他们没有把它称为办公室政治。

– plast1k
16-10-17在21:45



我同意您在这里说的话,而且这甚至在更远的地方,但是我认为重要的是要记住,代码审查不应具有对抗性。他们是两个人,他们的共同目标是编写好的代码和好的产品。有时可以就一种方法还是另一种方法达成共识,这是可以的,但是双方的所有观点都应植根于为团队,公司和/或客户做正确的事情。如果你们双方都同意这一点,那么这将是一个更顺利的过程。

–霍布斯
16-10-18在2:15

“除非它有一个简洁的例子,并且与所关注的问题直接相关,否则不要打扰挑选好的东西。” -我认为开放有点苛刻。当我进行代码审查时,我总是以“积极”的态度开始,从积极的方面入手,甚至我不得不诉诸良性的东西。它有助于设置基调,并表明您不仅仅是在寻找代码的负面方面。

–布莱恩·奥克利(Bryan Oakley)
16-10-18在2:55

“请确保您是在责怪代码而不是作者”。同意,但是不安全/不成熟的类型不会那样做。

–金属迈克斯特
16-10-18在13:18

#3 楼

代码审查可能是有毒的,浪费时间,会激怒活泼的书呆子战争。只需看一下对干净代码与注释,命名约定,单元和集成测试,签入策略,RESTfulness等等问题的意见分歧。写下通过代码审查的规则。

然后,不是失败或批准签到的人。他们只是在检查是否已遵守规则。

由于已预先记录下来,因此在编写代码时,您可以遵循规则,而不必在后面解释原因或争论。

如果您不喜欢这些规则,请开会修改。

评论


“确保避免这种情况的唯一方法是写下通过代码审查的规则。”这个。您应该根据为整个项目设定的一些标准来检查所有内容,而不是根据个人想法确定可行的方法,无论您的想法有多深刻。

–alephzero
16-10-17在21:27

问题是如何找到积极的东西。您怎么知道一个名字足够好?什么时候一个名字太穷而无法通过代码审查?他可能赞美的许多事情过于主观,无法一成不变地坚持下去。因此,我认为这不能回答问题。

–亚伦·霍尔
16-10-17在22:20

-1我喜欢您摆脱批评“书呆子之战”,然后说“确保避免这种情况的唯一方法”的方式。

– tymtam
16-10-18在0:26

不可能为每个可能的不良设计决策写下一条规则。而且,如果您尝试随手制作一个,很快就会发现该文档从纯粹的长度就变得无法使用。 -1

– jpmc26
16-10-18在2:52



比起编码标准,有用的开发人员和审阅者可以充当适当的成年人。

– gnasher729
16-10-18在14:23

#4 楼

我不会加糖,因为它可能被认为是光顾的。

我认为,最佳实践是始终专注于代码而不是作者。

这是代码审查,而不是开发人员审查,因此:


“此while循环可能永远不会结束”,而不是“您的循环可能永远不会结束” br />“方案X需要一个测试用例”,而不是“您没有编写测试来涵盖该方案”。

在讨论有关方案时应用相同的规则也很重要与其他人一起审查:


“安妮,您对这段代码有什么看法?”,而不是“安妮,您对乔恩的代码有何看法?”代码审查不是进行性能审查的时间-应该单独进行。

评论


您实际上并没有在回答问题。问题是“如何在代码审查中找到积极的东西?” -这个答案只是矛盾-您正在回答“我该如何给予负面反馈”。

–亚伦·霍尔
16-10-18在18:47

#5 楼

令我惊讶的是,到目前为止,在任何答案中都没有提到它,也许我在代码审查方面的经验是不寻常的,但是:
为什么要在一条消息中审查整个合并请求?
我在代码审查方面的经验是通过GitLab进行的;我一直以为其他代码审查工具也可以类似地工作。这极不可能被个人批评,因为它是对代码的注释,并且实际上是作为对代码的注释显示的,直接在要注释的代码下方显示。
我也可以对代码进行注释整个合并请求,并且经常这样做。但是可以在差异的特定行上指出特定点。 (此外,当添加新的提交以使diff发生变化时,默认情况下将隐藏对“过时的diff”的注释。)
通过此工作流程,代码审查变得更加模块化和内聚。来自代码审查的一行可以简单地说,

好方法,将其包装在专用函数中!对象名称与对象的用途并不完全匹配;也许我们可以改用“ XYZ”之类的名称?

或者如果某节存在重大问题,我可以这样写:我知道它为什么起作用),但是需要进行专门的研究才能理解它。您能否将其重构为单独的函数,以便将来进行维护?
(示例:函数ABC实际上在这里做三件事:弄糟foo,禁止boz和弄皱zorf。是单独的函数。)

完成上述所有操作后,我可以对整个合并请求进行摘要注释,例如:这是一个很棒的新功能,一旦合并,它将非常有用。您能否清理函数命名,并处理我所做的个别注释中提到的重构,然后让我知道再次查看它? :)


即使合并请求是完整的早餐,单独的注释也可以很简单。他们将更多。然后,摘要注释可能会说:

很抱歉,但是这段代码并没有真正使人窒息。在很多情况下(将在个别注释中详细介绍),这些情况会被错误地处理,并给用户带来不良的体验,甚至在一种情况下还会破坏数据。 (请参阅有关提交438a95fb734的注释。)即使是某些正常使用情况,也会导致极差的应用程序性能(具体说明参见somefile.c的diff注释)。更加注意在流程的每个点上可以安全地做出哪些假设。 (提示:除非经过检查,否则所有假设都是不安全的。)
我正在关闭合并请求,等待完全重写。


摘要:检查技术方面的代码作为对各行代码的注释。然后在合并请求的整体注释中总结这些注释。不要变得个人化,只需处理事实,并在您看来是关于代码的,而不是关于编码者的。并基于事实,准确的观察和理解来发表您的看法。

#6 楼

我真的很惊讶,没人能接受,但是发布的示例评论有问题。

没有理由直接与Joe联系。乔修复自己的失败与您无关。有人做是您的事。您关心的是代码质量。因此,不要写请求/命令/需求(如果我是Joe,我可能会因为您不合法而拒绝了),而是继续扮演自己的角色,并编写一个简单的匿名待办事项清单。

要公正地给出好与坏点不仅是不可能的,而且完全超出了您的角色。

以下是重新编写示例的内容,内容摘自您的评论:


在对Library \ ACME \ ExtractOrderMail类进行更改之前,我们需要修复一些问题。
除非我错过了一些内容,否则“ TempFilesToDelete”不应是静态的。我们可能每次运行都多次调用该函数,这就是为什么我们需要这样做(必须在此处完成)。 (而且,我现在处于临界点,因为到现在为止,您应该已经有了结论)
应该重命名SaveAndSend以更好地适应其行为,例如“ SendErrorMail”
注释掉的代码应该出于可读性目的而删除。我们使用Subversion进行最终的回滚。 。 WHO ?什么时候 ?没人在乎。什么 ?为什么呢应该说。

现在,您将通过消除人为因素来解决代码审查日益紧张的原因。

评论


样本评论是对该问题的最新补充,大多数回答者都没有看到它

–伊兹卡塔
16-10-18在23:47

#7 楼

代码审查的重点是发现问题。如果有任何错误,最好的办法是编写一个失败的测试用例。

您的团队(经理)应该传达出产生错误是游戏的一部分,但是要找到并修复它们将挽救每个人的工作。

召开定期会议(团队会议或双人会议)并解决几个问题可能会有所帮助。最初的程序员没有故意引入问题,有时他可能会认为它足够好(有时甚至可能)。但是,第二只眼睛给人的感觉是全新的,他可能会从解决这些问题中学到很多东西。

这确实是一种文化,需要很多信任和沟通。和时间来处理结果。

评论


“代码审查的全部要点是发现问题”,这是正确的-但这都不能回答所提出的问题。

–亚伦·霍尔
16-10-17在22:21



他在询问错误的xy问题,请参阅meta.stackexchange.com/questions/66377/what-is-the-xy-problem

– Eiko
16-10-18在8:01

您的团队(经理)应该传达出产生错误是游戏的一部分,但是找到并修复错误将挽救每个人的工作。这是事实,这意味着每个人都是利益相关者。但是,指出一个错误(或仅是不好的意大利面条代码)的人编写一个测试用例向原始编码器证明它是一个错误,不应该由别人负责。 (仅当人们普遍怀疑它确实是一个错误时才如此。)

–罗伯特·布里斯托-约翰逊
16-10-18在17:27

#8 楼

我认为积极的事情是在进行审查之前就编码标准达成共识。其他人在输入信息后往往会更多地购买某些东西。大多数开发人员会在同行之间达成共识。谁愿意成为不想同意编程世界中如此普遍的事物的人?当您通过选择某人的代码并指出漏洞来开始该过程时,他们会变得非常防御。建立标准后,在解释或“足够好”上会有分歧,但您的状况比现在要好。

此过程的另一部分是确定代码审查的方式将处理异议。这不能成为无休止的辩论。在某些时候,必须由某人做出决定。也许可以有第三方担任法官,或者审阅人获得所有权力。必须要做的事情才是目标。

这的最后一步是让所有人都知道代码审查不是您的主意,他们会留下来,所以每个人都应该努力充分利用它。如果每个人都感到无能为力,也许他们可以被允许查看您的代码?审查”,并认为,如果他们只是将其添加到您的流程中,就会在没有大量时间,精力和精力的情况下发生奇迹。

#9 楼

这可能很苛刻,但是如果没有什么可衡量的,那就不要担心会给出良好的反馈。

但是,在将来,随着您的开发人员开始改进其代码,那时候您将需要给他们很好的反馈。您将要指出代码中的改进之处,并且还要指出对整个团队的好处。当您开始看到改进时,它们也会一样,事情应该慢慢开始出现。

另一件事;可能会有防御气氛,因为他们觉得自己好像没有发言权。为什么不让他们检查彼此的代码?向他们提出具体问题,并设法让他们参与进来。不应该是你与他们对立;这应该是团队的努力。


如果有时间,您会对这段代码有什么改变?
如何改善代码库的这一部分?

现在问,然后问六个月后。这里有一种学习经验。

要点-不要在不需要保证的代码方面给予称赞,而要承认自己的努力并肯定会得到改进。尝试将评论作为团队练习而不是对抗性练习。

评论


“如果没有什么可衡量的,不要担心会给出良好的反馈”我很难相信他无法找到关于其他专业程序员编写的代码的正面评价,同时仍在提高每个人的期望码。这不能回答问题-这只是一个矛盾。

–亚伦·霍尔
16-10-17在22:24



@AaronHall:“您的代码可以作为一个很好的例子,说明如何不编写代码”。那足够积极吗?

– gnasher729
16-10-18在14:25

@AaronHall如果OP可以找到有关其他专业程序员编写的代码的肯定的话,那么他或她绝对应该这样做。但是,如果没有,那就没有必要去弥补。相反,OP应该专注于开发人员的努力和学习,而不是代码本身。

–午餐
16-10-18在16:07

#10 楼

没有张力的质量
您已经问过如何找到关于代码的正面评价,但是您真正的问题是如何在解决“严重的质量问题”时避免“ [您的]团队内的压力”。 >将“坏消息与好消息”夹在中间的老把戏可能适得其反。开发人员可能会从中发现它的实质:一种创新。
您的组织自上而下的麻烦
您的老板要求您确保质量。您想出了代码质量标准列表。现在,您希望向您的团队提供积极加强的想法。
为什么要问我们需要做些什么来使您的团队感到高兴?您是否考虑过问您的团队?
听起来您在尽力做到最好。问题不在于您如何传递消息。问题是沟通是单向的。
建立质量文化
必须培养一种质量文化,以便从下而上发展。质量提升得不够快,您想尝试应用《哈佛商业评论》的一些建议。
与您的团队会面。为您希望在团队中看到的行为建模:谦虚,尊重和对改进的承诺。最近遇到的生产问题。我个人认为我没有解决此问题。我认为我最大的错误不是一开始就没有更多的人参与。 [同事]和我仍然对代码质量的管理负责,但展望未来,我们将共同努力进行质量工作。”
从团队中获取有关代码质量的想法。 (白板会有所帮助。)确保您的需求到最后为止。以尊重的方式提供您的见解,并在适当时提出问题。愿意为您的团队感到很重要感到惊讶。保持灵活性,而不会损害业务需求。
(如果您有些尴尬的旧代码,则可以将其淘汰,鼓励大家坦率。最后,让人们知道您编写了它。模拟当其他人受到建筑批评时希望的成熟反应。 )
协作代码审查
我没有在您所描述的地方工作过,那里有一些高级程序员审查所有代码并进行更正。难怪人们会像您是老师在他们的论文上打上红色标记的那样回答。最好在小组中完成,包括您或其他负责任的开发人员。确保每个人都受到尊重。
同行评审代码的一个重要目标是确保团队的所有成员都能理解该代码。考虑一下您的函数名称不清楚的示例:比起初级开发人员,他们发现函数名称令人困惑,这比高级开发人员的另一项“更正”更容易接受。
质量是一个旅程要做的事情是消除所有认为代码审查是通过/失败的想法。每个人都应该期望在代码审查后进行一些编辑。 (而且您的工作就是确保这些事情发生。)
但是,如果那行不通...
假设您在建立质量文化方面迈出了一大步。您可能仍然无法吸引所有人。如果某人仍不参与质量计划,那么问题是他们不适合团队,而不是你们两个之间存在问题。如果需要从团队中解雇他们,团队中的其他成员将更好地了解原因。

评论


注意对等代码审查。我们做了一段时间,直到我们意识到一个初级开发人员对另一个初级开发人员进行了审查,并允许通过它的代码永远不会过去。团队中的两名前辈现在进行审查。

–古斯塔夫·贝特拉姆(Gustav Bertram)
16-10-19在14:16

#11 楼

很抱歉,还有一个很长的答案,但是我认为其他人没有完全解决这个问题的人为因素。

有时甚至只是问为什么要以特定方式实施某些事情。当我认为这很糟糕时,我指出我会用另一种方式来开发它。是您将开发人员立即置于防御状态。这个问题本身可能意味着各种情况,具体取决于上下文:您是否太愚蠢而无法想到更好的解决方案?这是您能做到的最好的吗?您是否要破坏这个项目?您是否还在乎工作质量?等等。问“为什么”开发代码的方式只会是对抗性的,这颠覆了您的评论可能具有的教学意图。对抗性的,因为开发人员立即想到的是“我是这样做的……您有问题吗?”再一次,它比需要的更具对抗性,并且使讨论不再改进代码。
示例:
而不是问“为什么选择不为此值使用常量变量?” ,只需声明“该硬编码值应替换为文件XYZ中的常量Constants.h”。该问题表明开发人员主动选择不使用已定义的常量,但是他们甚至可能完全不知道该常量存在。有了足够大的代码库,每个开发人员都不会有很多事情。对于开发人员来说,这只是一个很好的学习机会,使其熟悉项目的常量。
有一条很好的路线可以进行代码审查:您不需要为所讲的内容加糖,不需要将坏消息与好消息夹在一起,也不需要减轻影响。可能是我所处的文化,但是我和我的同事从未在代码审查中互相称赞过-我们开发的无缺陷的代码部分就足够了。您需要做的就是找出您看到的缺陷,并可能给出一个原因(当它很明显/简单时为什么不那么有用)。
好:


”需要更改此变量的名称,以匹配我们的编码标准。“


”此函数名称中的'b'必须大写才能成为PascalCase。“


“此函数的代码未正确缩进。”


“此代码与ABC::XYZ()中的代码重复,应使用该函数相反,“


”您应该使用try-with-resources,以便即使发生错误,也可以确保在此函数中正确关闭内容。 [我仅在此处添加了一个链接,因此非Java用户将知道try-with-resources是什么意思]标准。“


错误:


”我认为您可以通过更改变量名以符合我们的标准来改进此代码“


“也许最好使用try-with-resource来正确关闭此函数中的内容”


”查看此函数中的所有条件。它的N路径复杂度超过了我们标准允许的最大复杂度。“


”为什么缩进这里是2个空格而不是我们标准的4个空格?


“为什么编写一个违反我们n路径复杂性标准的函数?”


在错误的陈述中,斜体部分是人们在希望减轻打击时常用的东西,但是在代码审查中真正要做的只是使您不确定自己。如果您(审阅者)不确定如何改进代码,那么为什么有人应该听您的话?
“好”的陈述是直截了当的,但他们并没有指责开发人员,而是攻击开发人员,他们不是对抗性的,他们也不怀疑缺陷存在的原因。它存在;解决办法。他们当然没有最后一个“为什么”问题那样具有对抗性。

评论


您提供的许多示例都将通过静态分析检测到。以我的经验,代码审查中出现的事情通常更为主观和自以为是:“我之所以称X Y,因为我认为它可以更好地反映行为”。在我们的组织中,PR的创建者可以使用自己的判断并更改名称或保持原样。

–莫顿
16-10-19在12:38

@Muton我同意您的静态分析。在我从事的项目中,我们将这些检查自动化。我们还提供了可自动执行代码格式化的工具,因此大多数编码风格问题也不会(或很少)出现。但是OP特别提到他们的代码库很乱,所以我以为这是他们在审阅中要处理的问题,我认为这些简单的示例仍然与OP展示示例审阅所做的更新有关。

– Shaz
16-10-19在13:40

#12 楼

您看到的问题是:开发人员不满意他们的代码受到批评。但这不是问题。问题在于他们的代码不好(显然,假设您的代码评论很好)。

如果开发人员不喜欢他们的代码引起批评,那么解决方案很简单:编写更好的代码。您说您的代码质量有严重问题;这意味着需要更好的代码。

“为什么该方法应该有一个合理的名称,我知道它的作用?”我感到特别不安。他知道它的作用,或者至少他是这样说的,但是我不知道。任何方法都不仅应该有一个合理的名称,还应该有一个名称,该名称应使代码的读者可以立即清楚地知道其作用。您可能想要访问Apple的网站,并查找有关从Swift 2到Swift 3的更改的WWDC视频-进行了大量更改,只是为了使所有内容可读性更高。也许这种视频可以使您的开发人员相信,比他们聪明得多的开发人员会发现直观的方法名称非常非常重要。

另一个令人不安的事是您的同事,她说:“她告诉我自己,在客户提交错误报告后,她知道该错误,但担心开发人员会为此而生气。指出来”。可能会有一些误解,但是如果开发人员生气,如果您向他们指出错误,那就不好了。

评论


+1开发人员可能知道他的次优函数的功能,但是当他坐公交车时会发生什么?

–莫格说要恢复莫妮卡
16-10-18在7:57



#13 楼

我会很不同意您描述的代码审查方法。两名工作人员走进一个“封闭的房间”并受到批评,这使这种对抗性的观念制度化了,这种观念应该避免良好的代码审查。

使代码尽可能地回顾积极的经验对于使其成功至关重要。第一步是消除对抗性评论的概念。使其成为每周或每两周一次的团体活动;确保每个人都知道他们会受到欢迎。订购披萨或三明治或其他任何东西。如果这会激起负面的含义,甚至不要称其为“代码审查”。寻找一些值得庆祝,鼓励和分享的东西-即使只是通过当前的sprint或迭代取得的进展而已。轮流指派领导进行审查。

使这一过程努力为产品和人们服务。好处。每个人都在公共场合接受教练。如果您有一个问题程序员没有“得到它”,则应该私下单独解决它们-切勿在更广泛的团队面前。这与代码审查是分开的。

如果您找不到要提出的“好”东西,那对我来说是一个问题:如果项目正在取得进展,并且人们正在努力,则该进展本身值得庆祝。如果您真的没有发现任何不好的地方,则意味着自上次审核以来所做的一切都是不好的,或者没有比中立更好的了。是真的吗?

关于细节,共同的标准是必不可少的。让每个人都在做什么。并允许将更新,更好的技术集成到您的代码库中。不能保证“老习惯”的行为永远不会以“我们一直那样做”的幌子来消除。

所有这一切的关键是,由于代码审查的实施不当,它们给代码审查带来了不好的效果,被用作贬低经验不足或技能欠佳的程序员的工具,这对任何人都没有用。以这种方式使用它们的经理也不可能是非常有效的经理。良好的代码评审,每个人都是参与者,为每个人(产品,员工和公司)服务。

很抱歉,文章的长度,但由于经常被忽略代码审查,这导致了无法维护的代码的遗留问题,即使允许开发人员,也只有极少数开发人员能够这样做更改为已有多年历史的代码库。我选择不再走这条路。

评论


+1可以亲自而不是通过电子方式进行代码审查-这将消除批评

–alexanderbird
16-10-18在21:37

#14 楼

好的代码的矛盾之处在于它根本不突出,并且看起来非常简单易写。我真的很喜欢这个答案中的台球玩家例子。在代码审阅中,这真的很容易掩盖,除非它恰好是不良代码的重构。如果我正在查看代码,并且浏览了一个非常容易阅读的文件,看起来似乎很容易编写,那么我会快速抛出“我喜欢这里的方法简短明了”或任何合适的方法。

此外,您应该以身作则。您应该坚持也要检查您的代码,并且应该对希望团队进行纠正时的行为建模。最重要的是,您应该衷心感谢大家的反馈。这比您可以提供的任何单方面反馈的影响更大。

#15 楼

听起来真正的问题是,只有两个人在进行所有代码审查,只有您自己认真对待它们,这使您陷入了不幸的境地,承担了很多责任,来自其他团队成员的压力。

更好的方法是在团队中分配进行代码审查的责任,并让所有人参与其中,例如让开发人员选择谁查看他们的代码。这是您的代码检查工具应支持的东西。

评论


不确定为什么要投票(没有评论的投票在关于良好代码审查的线程上很愚蠢)。每个人的审查都应该是标准程序。在看板板上,将有一个专栏进行代码审查,并且团队中的任何人选择下一个项目都应进行审查(警告:新手不应在一段时间内接受审查,然后应从需要领域知识很少)。在混乱板上,基本上类似:从右到左工作。

– Dewi Morgan
16-10-21在15:39



#16 楼

我已经亲眼目睹了这种情况的发生,并想提醒您,不要接受情绪智力挑战的答案-它们​​会杀死整个团队。除非您想每年招募,培训和规范新的开发人员,否则与同行建立积极的关系至关重要。毕竟,难道不是为了提高代码质量并培养一种首先提高代码质量的文化而进行这些审查的全部目的?我认为,提供积极的强化确实是您的工作一部分,以将该代码检查系统集成到团队文化中。

听起来,您似乎需要检查您的代码检查系统。现在,从它的声音来看,您审查过程中的所有内容都是主观的,而不是客观的。当感觉某人只是因为他们不喜欢别人而拆开您的代码,而不是因为他们有理由在某些不符合准则的情况下可以引用的理由时,很容易感到受伤。这样,就可以以适合您办公文化的任何方式轻松跟踪和“赞扬”代码质量的积极改进(相对于您的审阅系统)。

技术负责人需要在审查会议之外聚会,并制定《编码指南/代码审查清单》,并且在审查期间必须遵守并参考。这应该是一个实时文档,可以随着流程的发展而更新和完善。这些技术负责人会议还应该在“我们一直做事的方式”与“新的和改进的做事方式”讨论之间进行,而无需对开发人员进行审查,就像他们的代码导致分歧。一旦初步指导原则或多或少地得到了完善,积极增强开发人员的另一种方法是征求他们的反馈,然后对之采取实际行动,并以团队的形式不断发展,这确实使我们加快了开发速度。开始审查板载代码,因此您也不会退休,也不会停下来进行审查。

#17 楼

前提...

程序员是解决问题的人。当出现问题或错误时,我们有条件开始调试。

代码是大纲格式的介质。切换到段落格式的叙述以获取反馈会导致必须断开连接。不可避免地会丢失或误解某些东西。不可避免地,审稿人不是用程序员的语言讲话。

很少会质疑计算机生成的错误消息,也不会将其视为个人冒犯。生成的错误消息。它们旨在捕获程序员和自动化工具遗漏的极端情况以及对其他程序和数据不可避免的副作用。

结论...

因此,可以将更多的代码审查项目合并到自动化工具中,则它们收到的效果会更好。

告诫,为了毫无疑问,此类工具必须每天或每周不断更新,遵守对程序,标准和惯例的每次更改。当经理或开发团队决定进行更改时,必须更改工具以强制执行更改。然后,代码审查员的大部分工作便变成了维护工具中的规则。

示例代码审查反馈...

使用以下技术重写了给定的示例:




主题:


库\ ACME \ ExtractOrderMail类。


/>
原理问题...


TempFilesToDelete是静态的


对GetMails的后续调用会引发异常,因为文件已添加它,但删除后再也不会删除。尽管现在只有一个调用
,但是将来可以通过某些
并行性来提高性能。
TempFilesToDelete作为实例变量将允许并行使用多个对象。




次要问题...


GetErrorMailBody具有异常参数


由于它本身不会引发异常,而是将其传递给ToString,是否有必要?


SaveAndSend名称


将来可能会或可能不会使用电子邮件来报告此事件,并且此代码包含用于存储持久性副本和报告任何错误的一般逻辑。一个更通用的名称将允许这种预期的
更改而无需更改相关方法。一种可能是
StoreAndReport。


,但“注释墙”也会掩盖相邻代码中的错误
。我们在Subversion中仍然有它。也许只是一个
注释,确切地引用了Subversion中的位置?

#18 楼

如果您对现有代码没什么好说的(听起来很容易理解),请尝试让开发人员参与其中的改进。将其他更改(重新命名,重构等)捆绑到同一补丁中。它应该进行修复该错误或添加功能的更改,仅此而已。



这很丑陋,但我想它与我们所有其他讨厌的代码一致。让我们稍后进行清理(理想情况下,在补丁中不进行任何功能更改)。它使您有机会说出为什么认为某事是好事而不是坏事,并且还展示了一个很好地接受建设性批评的例子。一个新功能,很好,它们位于主补丁中。但是,如果您要修复错误,则测试应在先前的补丁程序中进行,并且不能通过,因此您可以演示该修复程序实际上已修复了该错误。 ,或重构内容和时间)应该在工作完成之前进行讨论,这样他们在发现问题之前就不会花很多精力。

如果发现代码无法解决您认为应该输入的内容,也可能与开发人员认为合理的输入不匹配。如果可能的话,也要事先同意。至少对它应该应付的内容以及如何允许它失败的讨论有一些讨论。

评论


无论是否在单独的补丁程序中,都需要使用旧版代码恢复破损的窗口策略,无论该策略是“不修复破损的窗口”还是“仅在当前补丁程序涉及的[方法/类/文件? ”。以我的经验,阻止开发人员修复损坏的窗户对团队士气有害。

– Dewi Morgan
16-10-21在15:44

是的,但是强迫他们在单行更改通过审核之前修复模块中的每个损坏的窗口同样有毒。

–没用
16-10-21在15:45

同意!我认为,由团队制定而不是从外部实施的政策是唯一可行的政策。

– Dewi Morgan
16-10-21在15:50

#19 楼

我认为假设有一个技术或简单的解决方案是错误的:“当我按我的标准判断他们的工作时,我的同事感到沮丧,并有权执行它。”

记住代码审查不只是讨论什么是好是坏。它们隐含地是“我们正在使用谁的码尺”,是“谁来做决定”,因此(最阴险地)是排名。

如果您不解决这些问题,请在没有遇到的问题的方法将很难。这里有一些很好的建议,但是您已经为自己设定了一项艰巨的任务。

如果您是对的,没有任何回旋余地,指出这是一种攻击:有人搞砸了。如果不是:您的另一所MBA学生却没有。无论哪种方式,您都将成为坏人。您不是网守,而是校对员。获得此协议是一个很难解决的问题[1]。我想给您提建议,但我自己还没有掌握...

[1]这还不能因为人们说“好”或停止战斗而得到解决。关于它。没有人愿意说X对于我们的{智力,经验,人力,截止日期等}不切实际,但这并不意味着实际上涉及到某些特定的X实例...

#20 楼

代码审查应该是相互的。大家都批评大家。让我们的座右铭是“不要生气,要平庸”。每个人都会犯错,一旦人们告诉了如何解决他们的代码的热点,他们就会明白这只是正常的过程,而不是对他们的攻击。了解代码以至于可以指出其弱点,并必须说明弱点可以教给您很多东西。寻找缺陷。但是,您可以在修补程序上大加赞赏。及时性和简洁性都值得称赞。

评论


不确定为什么要投票(没有评论的投票在关于良好代码审查的线程上很愚蠢)。每个人的审查都应该是标准程序。在看板板上,将有一个专栏进行代码审查,并且团队中的任何人选择下一个项目都应进行审查(警告:新手不应在一段时间内接受审查,然后应从需要领域知识很少)。在混乱板上,基本上类似:从右到左工作。

– Dewi Morgan
16-10-21在15:41

@DewiMorgan我不同意“新手不应在一段时间内接受评论”。新手进行审阅是让他们熟悉代码库的绝佳方法。也就是说,他们不应该是唯一的评论者!就是说,无论如何,我还是很警惕,大多数时候只有一名审稿人。

–幻灭
16-10-25在13:55