这是一个创建于 988 天前的主题，其中的信息可能已经有所发展或是发生改变。
背景
最近团队正在用 Java 重构 PHP 老项目（底层服务），陈年老代码，难度很大。因为测试资源有限，所以在开发时对于质量的标准就很严格。 基于此，团队达成了共识：要花点时间写单元测试和进行严格的 CodeReview 。
情况
在执行过程中呢，关于具体的 CodeReview 和单元测试的实践上，我和团队小伙伴产生了分歧。我理解的 CodeReview 是应该少量多次，每天提交代码时互相 Review 一下，人也轻松。而一些团队小伙伴认为，在提测之前，进行一次大而全的 Review 就可以了。包括对待单元测试的态度也差不多。 关键的点在于，我的个人想法是：如果不能频繁地做 Review ，那么等到最后，可能会迫于交付的压力，对着写了好几个月的代码望洋兴叹，最后就是应付交差，后期改问题的时间也会很长。而小伙伴们似乎对这个悲观预期不是很理解，我也无法说服他们。
结果
在一番讨论后，我们勉强达成了个共识，以模块开发完成为节点进行 Review ，也就是在数月的开发过程中尽量多次 Review ，既不是最后提测统一做一次，也不是每天做。只是这个结果我仍然悲观。
求助
不知道大家有没有遇到这种关于工程实践的分歧争论的情况，有没有什么良好的解决方案呀。自己默默 emo 中
37 条回复 • 2022-03-19 11:57:25 +08:00
 |
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 。当然这种对于一般企业项目来说有点极端了，大概也是很多开源项目喜欢鸽的原因之一吧。 |
Select Language