最近团队正在用 Java 重构 PHP 老项目(底层服务),陈年老代码,难度很大。因为测试资源有限,所以在开发时对于质量的标准就很严格。 基于此,团队达成了共识:要花点时间写单元测试和进行严格的 CodeReview 。
在执行过程中呢,关于具体的 CodeReview 和单元测试的实践上,我和团队小伙伴产生了分歧。我理解的 CodeReview 是应该少量多次,每天提交代码时互相 Review 一下,人也轻松。而一些团队小伙伴认为,在提测之前,进行一次大而全的 Review 就可以了。包括对待单元测试的态度也差不多。 关键的点在于,我的个人想法是:如果不能频繁地做 Review ,那么等到最后,可能会迫于交付的压力,对着写了好几个月的代码望洋兴叹,最后就是应付交差,后期改问题的时间也会很长。而小伙伴们似乎对这个悲观预期不是很理解,我也无法说服他们。
在一番讨论后,我们勉强达成了个共识,以模块开发完成为节点进行 Review ,也就是在数月的开发过程中尽量多次 Review ,既不是最后提测统一做一次,也不是每天做。只是这个结果我仍然悲观。
不知道大家有没有遇到这种关于工程实践的分歧争论的情况,有没有什么良好的解决方案呀。自己默默 emo 中
1
coolzjy 2022-03-18 10:46:18 +08:00 5
你的想法是理想,小伙伴的想法是现实。
|
2
zzfer 2022-03-18 10:53:08 +08:00
别的不知道,互相 Review 效果挺差的。除非有一套人人都认可的标准且你们所有人都能认真执行。
|
3
liuzhaowei55 2022-03-18 10:53:14 +08:00 via iPhone
提高单元测试覆盖率吧,cr 在我看来大部份时间只能找出来一些语言层面的问题,对业务上的细节覆盖不到,除非这些业务细节大家都很熟悉。
|
4
maichael 2022-03-18 11:00:26 +08:00
在这方面来说没有一步到位的方法,总是先定个框架,然后再慢慢根据真实情况调整。
Code Reivew 最重要的是定标准,不然的话总会在 Review 过程中产生分歧,导致时间都浪费在这上面。 另外,说服其实不难; Review 一两百行的更改和 Review 一两万行的更改,你更愿意 Review 哪一个。更不要说后者这种长时间段的改动,往往写代码那个人都会忘记很多其中的细节,Review 的过程中有很多时间都会浪费在这上面。 |
8
iyaozhen 2022-03-18 11:03:48 +08:00 1
个人认为 CR 还是非常有用
我是 QA ,RD 的代码我都会 CR 。 我对业务还是相对熟悉(这个前提很重要),CR 不是发现语法问题,这些静态代码检查就行了,主要是发现业务逻辑问题,还有实现的问题,实现不能得过且过,还是能发现很多问题的 还有个作用就是促进相互了解,两个人开发模块,可能都重复造轮子了,互通一下比较好。还有就是你以后也会开发别人的模块,相互了解团队人员也能相互 backup 其余还有个好处就是大家写代码会多想一点,把代码写好。我们团队之前就遇到一个模块的代码 review 了一星期还没合进去,有了这次经历后续那个 rd 写的代码质量就好很多了。(当然这个看人,说不定别人还恨你) 最后频率问题 「每天提交代码时互相 Review 一下,人也轻松」你这样不太好,代码不是按行或者按天分的,今天写的逻辑都不完整,而且每天提交也不是个好习惯。应该是能运行的一个子功能提交一次( bugfix 除外),基于此 review 。提测前可以再拉上 QA 一起 review 下 |
9
xuboying 2022-03-18 11:07:15 +08:00
OP 提的 Review 是已经去除 LINT 等机器能自动扫描以后,单纯的人工 review 部分么?
|
10
Rwing 2022-03-18 11:11:50 +08:00
尽量把 codereview 放在 pull request 里,也就是每次的 pr 必须要 review 才能 merge
|
11
MiniGhost 2022-03-18 11:17:40 +08:00
你的观点是正确的,少量多次肯定是好的
我之前遇到过一些同事,一口气提一两千行代码,这是指望 Reviewer 是个神仙吗? 错误的习惯就应该就改正,而不是迁就他 还有就是,这种事情给我的启发跟教训就是,如果不是很好的团队,就不要搞民主,搞专政搞一言堂。 先把自己的方案推下去,日常多留心一下大家的执行情况,团队里面人员有执行不到位的即时纠正,之后再看情况了解了解大家的反馈,是否要做一些调整 |
12
yzbythesea 2022-03-18 11:20:58 +08:00 1
无法理解你的看法。确保代码质量不应该是依赖单元测试,整合测试,Canary 测试这些客观手段进行吗?第一过度依赖 CR 本身就不靠谱;第二 CR 质量和 CR 大小没有关联;第三无效的规章制度定得太多,会影响人家工作舒适感(就和打卡上班一样)
|
16
longgediyi999 2022-03-18 11:35:46 +08:00 2
让摸鱼的小伙伴无所遁形 一天了 啥也没写
|
17
RiceNoodle 2022-03-18 11:45:01 +08:00
团队一直在做 Code Review ,说一点我觉得好的方式
1. 功能完整再去做 review ,太零碎的 PR 进行 code review ,很难找到一些逻辑性或者结构性的问题。 2. 最好有个主程的角色,或者某个业务主程的角色,由他决定是否能够 approve ,允许 merge 。一票否决,这样能够保持某个功能模块,有统一的标准。 |
18
smilenceX 2022-03-18 14:01:02 +08:00 via Android
@yzbythesea 非常赞同。
|
19
C603H6r18Q1mSP9N 2022-03-18 14:19:42 +08:00
我们以前是(以前是):每天早上站会,团队成员可以选择 今天 review 或者后面,但是每个功能总会 review 一遍,review 的时候可以看写的有没有问题或者漏洞,有没有更好的写法等,每次 review 都是一步小成长
|
20
niceyuri OP @shanghai1998 这个办法应该团队小伙伴也能接受~
|
21
MiniGhost 2022-03-18 14:49:03 +08:00
@yzbythesea #12 @smilenceX #18
我对此持相反观点,不知道你听没听过 “代码是用来让人读的,只是顺便让机器执行而已” CodeReview 不是为了保证 0bug 。比如一些边界值、异常情况没有考虑,直到测试阶段才暴露出来这太正常了。 CodeReview 最要求是更简洁、更已读、更适合的代码,比如是否落实了项目规范中的 MVC 、DDD 、比如是否 3 行代码就可以搞定的事情但是你不知道你写了 30 行、比如是否满足了 SOILD 原则等等。 简而言之:Code Review 是用来保证代码质量,测试是用来保证产品质量,这两者并不是一个东西。 |
22
k9982874 2022-03-18 14:57:33 +08:00
你们的流程就有问题,按工单提 PR ,按 PR 去 review ,一个 PR 两人以上 approval 就可以合并,人不够的小团队由技术老大把关
|
23
smilenceX 2022-03-18 15:22:53 +08:00 via Android
@MiniGhost
我前面赞同的是“保证代码质量依赖的是各种测试手段“,以及“无效的规章制度影响工作舒适度” 我在打上一个回复是混淆了代码质量和产品质量的概念,可能因此引起的你的误解。 总的来说,我们的看法应该没有冲突。 |
24
smilenceX 2022-03-18 15:25:17 +08:00 via Android
@smilenceX
更正错别字 我前面赞同的是“保证代码质量依赖的是各种测试手段”,以及“无效的规章制度影响工作舒适度” 我在写上一个回复时混淆了代码质量和产品质量的概念,可能因此引起了你的误解。 总的来说,我们的看法应该没有冲突。 |
26
libook 2022-03-18 15:34:12 +08:00
我的团队一开始也是大而全的 review ,但是 review 出问题之后是需要改的,改完后还需要再次 review 。
试行一段时间之后团队成员反馈每次 review 时间很长,需要数小时甚至一两天,比较煎熬,而且大多情况下问题是连锁式的,早期一个问题解决后就不会出现后续一系列的问题。 后来我们就更换为每天一次 review ,每天下午下班前,6 人团队每人不到 10 分钟给大家讲解当天写的代码,然后其他成员一起 review 、提出问题。 |
27
golangLover 2022-03-18 15:36:26 +08:00 via Android
少量你才能看懂,不然大量交上去,一次看两个小时的你们会喜欢看?最后就荒废了
|
28
yzbythesea 2022-03-18 15:56:11 +08:00
@MiniGhost
我指的“代码质量”里面最重要的环节就是 0 bug (或者你讲的产品质量),也包括你认为的狭义的“代码质量”,比如 coding best pratice ,OOD ,可阅读性等。但实际工作中(个人经验),CR 在处理狭义代码质量上作用不大,因为我觉得比如 OOD 这是基本素质吧,一般只有应届生需要更多提醒下。 |
29
tool2d 2022-03-18 16:19:57 +08:00
@MiniGhost "CodeReview 最要求是更简洁、更已读、更适合的代码"
世界上没有银弹,没有所谓简洁的代码。 只要项目需求复杂,代码就会随着时间推移,不断变复杂,和熵增原理一样。 定期重构可以缓解这一现象,但这时候 CodeReview 的重要性就下降了。因为你上次 review 的代码,下次很可能直接就被删掉了。 |
30
joesonw 2022-03-18 17:14:13 +08:00 via iPhone
review 不是批判别人的代码风格,不然量大,大家也闹的不开心。而是找出遗漏的低级错误,或者大家对于这个功能点理解不一致的交流。然后就是交叉熟悉代码,不然负责这块的请个假的时候正好要改不熟悉的话就麻烦大了。
|
31
sampeng 2022-03-18 17:25:39 +08:00
我弱弱的问一句??和小伙伴自行沟通? leader 跑哪去了?
标准是什么有定么?尤其是 java ,一个事情,多个抽象模型都是可行的。标准是什么? 比如 检查重复代码 检查业务逻辑可读性(就是真的看懂了,注释是否合理,满屏幕注释是什么鬼) 检查单元测试覆盖是否合理 review 重要逻辑实现方案是否合理 等等。。要先有标准再有 CR 。盲看?强制把所有人拉在一个水平线是不现实的事 |
32
MiniGhost 2022-03-18 17:32:43 +08:00
@yzbythesea #28
我刚刚讲的只是举了几个在 Code Review 中关注的例子,并不全,我还是认为即使稍有一定工作经验的人代码也不一定就是 OK 的... 也许我们之间对代码 OK 的标准的理解存在差异。 比如要求实现一个订单支付的接口,不同的人也许会有不同的设计方式: - POST /order/{order_id}/payment - POST /order/{order_id}/pay - POST /order/pay?order_id=xxx 也比如有的人曾经的工作经验是 HTTP Response 在程序中永远反 200 ,也有的人会相对更遵守 RESTful 。 这上面的取舍,在产品层面都无关产品质量,但是我认为这应该也是 Code Review 时应该把控的一部分。 也有可能一些团队对这些也有明确的约束,但是 Code Review 还是有很多东西值得关注的。 我日常中还有的一些 Code Review 纠正同事的案例还有: - 这段写的多余了,我之前写过类似的,在 xxx ,你复用一下就可以了 - 这里可以使用语言的新特性、xxx 第三方库实现,更简洁 - 标准库里有标准实现,不用自己再写一遍,直接 xxxx 就完事儿了 - 这里用防御式写法,先把这几种异常情况先判断处理了,剩下的代码逻辑会更干净 还有很多是语言相关的,不清楚你主语言是什么,怕讲出来非这个语言的开发 Get 不到点就不写了。 |
33
MiniGhost 2022-03-18 17:45:56 +08:00
@tool2d #29
第一句话我就不认同,我认为必然有简洁的代码... 举个例子,写个排序,我是自己手写个排序算法简洁,还是直接 array.sort(list) 简洁? 再聊随着需求增加,会不断熵增,这个我完全认同,重构我也能接受。 但是你认为,重构屎山简单,还是重构相对清晰简明的代码简单? 很有可能在熵增的过程中,把控不好,产品层面熵增了 O(n),代码层面熵增了 O(n²),Code Review 做得好可以尽可能让业务熵增与代码熵增呈现一个线性关系。 |
34
chocolate518 2022-03-18 18:00:23 +08:00
其实实际上可能投入测试是最实际的解,单测不是为了发现 bug ,cr 也不会降低 bug 。
|
35
night98 2022-03-19 02:25:20 +08:00
先问个问题,你们老大呢?
|
36
night98 2022-03-19 02:34:35 +08:00
补充一下回复,关于 code review ,比较好的做法是完成一个业务功能提交一次,单次 review 尽量保持在 300 行以内,以 Java 为例,使用 mybatis 或者 jpa 对数据表进行建模生成 model 后并添加对应枚举后,提交一次 review ,这次 review 速度一般比较快,基本上都是按团队规范和机器自动生成的,没啥太多 review 的地方,最多检查下是不是建模有问题,一般一两分钟搞定。接着就是常规的功能 review 了,通常建议拆分为业务功能多次提交,以最常见的订单系统为例,会拆分出例如订单分页接口,订单详情接口,订单提交接口,这三个接口分别提交三次 pr+review ,主要目的是保持 review 的单个逻辑连续性,如果单次提交在 1000 行以上,作为 review 评审者,其实相对难度较大,通常还需要 clone 到本地详细查看,成本太高,可能就直接点通过省事了,关于 review 其实从出发角度来看,其实是为了提升整体代码质量,避免代码崩坏,减少后续维护成本。比如团队如果有实际的代码规范的话,reviwe 主要就是关注代码规范,加上潜在 bug 发现,以及一些常规的代码质量改进,例如可以用新特性或者其他更好的方式去实现。关于你提的这个问题其实我觉得也挺有意思的,其实还是那个问题,你们老大呢,工程实践这玩意说实话就是有和没有在短期没什么太大差距,都是长期来看才有价值,否则也没有那么多的人会写垃圾代码了
|
37
secondwtq 2022-03-19 11:57:25 +08:00
我觉得很重要的一个因素是你们有多少资源。
比如项目很紧,那不 review 也无所谓(最后再 review 跟不 review 也没啥区别了 ...) 比如我们 ddl 不紧,现在是模仿开源项目的模式,每个 commit 都 review ,Code Review 是日常工作内容之一。但是效率就低了。 楼里觉得 Code Review 意义不大的可以思考一下为什么很多知名开源项目都在做 Code Review 。当然这种对于一般企业项目来说有点极端了,大概也是很多开源项目喜欢鸽的原因之一吧。 |