“从前我们进行了代码审查,在邮件中写了行号。这很有意思。从专业人士的角度来看:在IDE中,没有人看过差异。但也有一个缺点:合并后,行号改变了。”
亚历山大·马卡罗夫(Yii)
“我们公司有一个有趣的概念-要求提供椅子。这是在同一办公室内,开发人员坐在椅子上向您说:“看,这比创建拉取请求要快。”
WormSoft的Anton Morev
最近,SDCast播客公开录制了有关YouTube上的代码审查的记录。我们从问题中选择并解读了最有趣的内容。
Spotify,Ya。Music上的完整音频版本以及要下载的文件,Youtube上的
完整视频版本
不要将代码审查像检查某些东西或查找错误一样
Sergey Zhuk,Skyeng:我相信代码审查的基本目标是在团队内部共享知识并找到最佳解决方案。团队将审查建议的更改-有关该域的知识将平均分配给其成员。该解决方案的作者理解他认为完美的代码实际上可以得到改进。
因此,代码审查不是要避免或更快完成的事情。这是对业务和团队的投资:代码变得更好,团队变得更好。是的,起初我不喜欢请求被打包时(谁愿意)。
但是后来我开始将其视为学习过程,与阅读书籍或参加会议相提并论。
作为一名开发人员,我从长于这种方法,对此我感到很高兴。
但是有细微差别。当然,您中的许多人都遇到了数千行的请求,其中包含重构,功能和一些小的更改。通过在一个请求中混合不同的内容,我们使知识共享和审查者的生活变得复杂:他将很难评估系统的行为是否已更改,业务需求是否得到满足,整个问题是否已得到解决-解决方案是否成功?
我们注意到了团队中的这一刻,并引入了一条规则:在一个请求请求中,请勿干扰其他性质的更改。您分别发送重构,单独功能和单独小的更改。
谢尔盖(Sergey)关于团队实践的报告。文本版本在这里。
这些请求通常可以快速,轻松地进行审核,并且更有可能收到质量反馈。在维护者方面,还有一些优点:如果您喜欢重构,并且该功能存在错误,则可以分别冻结重构。
Alexander Makarov:我同意您不应尝试将尽可能少的时间花费在代码审查上。打开后,就像方括号一样,此代码可以执行某些操作-不能那样工作。如果审阅者无法保证最终代码,则意味着尚未执行代码审阅。
因此,我们现在已经开始为自己编写大量准则,其中之一是关于代码审查的。
部分的Yii的团队的指导方针。
但是关于个别小拉动要求的论点是:我遇到过这样的情况,即领导来了,并介绍了类似的东西。团队充满敌意。你怎么得到的?
谢尔盖·朱克(Sergey Zhuk):有一个与我们并行互动的团队,使用了他们的API。他们在一个月内完成了很多功能,而我们做了一点。就是说,起初我们不关注功能的交付,而是关注这种方法的质量。我们同意业务,我们将更慢地发布它,但我们尽量不破坏任何东西。一个月后,一个邻居崩溃了,然后又一个崩溃了。但是我们没有。在这个例子中,每个人都理解了一切。
康斯坦丁·布尔卡列夫(Sonta Konstantin)我一般都有实现代码审查的流程-这并不容易,尽管每个人似乎都知道这很好,是的。您说,“伙计们,让我们通过拉取请求合并。” 他们对您说:“是的,有一个小功能。”
该原则在这里非常有效,当出现问题时,您会说:“但是,如果您提出了要求,我们会进行调查的,只是没有实现。” 这个想法是让人们从自己的经验中了解错误。施加强制并非总是可行。
审查速度
在广播期间,观众之间进行了投票。这是其中之一。
康斯坦丁·伯卡列夫(Konstantin Burkalev):6月通常是这样的:“好吧,您看着我的要求,可以吗?我写下来,握紧拳头,然后等待”。
我有我自己的任务,我只能在晚上到达,否则我
一无所知。谢尔盖·朱克(Sergey Zhuk):在我们的团队中,评审始终是重中之重。因此,有一条规定-当请求请求到达时,您要完成当前的操作,以免丢失上下文,然后进行复查。因为您将要进行的工作仍在进行中。这项任务已经完成。
该代码已经编写完毕,仅用于查找,合并和上载到产品中。
也就是说,我完成了自己的工作,切换,快速浏览并继续工作。大概每天3次,我会分心于检查任务。但是,再次,您必须了解,在我的团队中,所有内容都分为小请求请求,每行200-300行代码。因此,花费最少的时间。
“谁比谁重要”
康斯坦丁·伯卡列夫(Konstantin Burkalev):这就提出了一个问题-谁应该审查。仅铅?只有参议员?还是可以并且应该给予能力较低的人,以便一个人努力成长?
当被问到要回顾什么时,人们会这样回答。
亚历山大·马卡罗夫(Alexander Makarov):如果您的团队中有很多人,并且领先优势造成了自己的瓶颈,那么这会减慢整个团队的速度,从而使团队变得更加糟糕。作为负责人,当我在Skyeng工作时,每天在高峰期有15个请求,而不是最小的请求。我在早上和晚上预留了时间进行审查。
有必要对所有人进行审查。也许除了那些“着火了,一切都着火了”的故事-那里的情况不会更糟。
我搞砸了,没关系-即使我已经使用同一项目很多年了。因此,现在我正试图邀请最多的人观看我的请求。这很好,应该如此。
每个人是否都应该信任评论是另一回事。我有些人可能会认为很棒,但他们在重点方面有很大的问题:可以说,一个人花了6个小时进行审查。我不得不教人们如何管理自己的时间。
如果我们谈论的是商业公司,我赞成指定谁负责这项责任,谁可以随意进行审核-以及每天可以花多少时间根据此状态进行审核。
安东·莫雷夫(Anton Morev):如我所见,有不同的过程。如果我们正在做一些需要在短时间内发布的功能,则我们不能让June看到代码。但是,如果我们正在做某种内部产品,或者截止日期不高,是的,让June回顾首席或高级开发人员的工作是一个好习惯。因此,Juns将更好地了解整个项目中正在发生的事情以及决策机制是如何工作的。
“这是立即拒绝”
Sergey Zhuk: Skyeng对提交消息进行了检查:JIRA中必须有一个任务号,否则合并请求不能合并。有时,您打开代码,只是不了解其中的内容-打开任务就可以理解。
对于其他情况,一开始对我们来说很艰难,然后我们决定仅在任务完全错误或团队在架构上不同意时才拒绝它。依此类推:我们批准,合并,写评论-如果请求请求的作者想要,他将返回并修复它。在那里,他将校正较小的粗糙度或使用某种图案。您的拒收行为是什么?
亚历山大·马卡罗夫(Alexander Makarov):标准完全符合您的条件-如果任务做得不好,那么您必须将其总结一下。即使代码看起来不错并且在体系结构上一切都很棒。
, : , .
例如,我们说:“伙计们,让我们进行测试”。当然也有例外,但是测试非常重要。从他们中可以清楚地知道该人是否正确理解了该任务:再次,如果他正在测试类和方法,而不是用例,则这已经是可疑了。我们现在搞砸了感染,这是突变测试。 Tulza保留了相同的测试,但是修改了代码并运行。如果更改后的代码通过了相同的测试,则不需要部分代码-您可以将其删除,删除,什么都不会改变。
有点后台。
另外,当然还有安全和性能问题-在这里会遭到拒绝。我们不会拒绝琐碎的事情:要么是我们要求对其进行修复,要么我们自己对其进行修复-我们直接将这些问题提交给提出要求的人员,而我们已经在完成的代码中说明了实现的方式,方式和原因。
安东·莫雷夫(Anton Morev):关于你所说的-问题解决了吗?碰巧的是,开发人员在解决一个问题的同时解决了另一个问题。不必拒绝这种情况。或者至少弄清楚任务是如何进行的。
康斯坦丁·伯卡列夫(Konstantin Burkalev):但是,将提交消息与任务跟踪器链接的时刻对我而言似乎很重要。在日常工作中,它有很大帮助。您是否知道何时:“听着,我们在按钮问题上做了类似的事情。” 您找到有关该按钮的任务,了解其编号,转到存储库日志,找到那些提交的差异,然后查看:确实,我们已经搞定了这一点,可以将其移到此处。
但是相反的情况也会发生。我正在查看一个项目的源代码,并且在后端遇到了action684函数。
我问一个朋友,代码中这个很酷的名字是什么。他回答-对跟踪器中任务的引用。“为什么要为函数命名,却是将其作为任务的一部分来编写的。” ... Thresh,当然在正常情况下不应该存在。