关于 CodeReview，和团队小伙伴产生了分歧。

[niceyuri]

背景

最近团队正在用 Java 重构 PHP 老项目（底层服务），陈年老代码，难度很大。因为测试资源有限，所以在开发时对于质量的标准就很严格。 基于此，团队达成了共识：要花点时间写单元测试和进行严格的 CodeReview 。

情况

在执行过程中呢，关于具体的 CodeReview 和单元测试的实践上，我和团队小伙伴产生了分歧。我理解的 CodeReview 是应该少量多次，每天提交代码时互相 Review 一下，人也轻松。而一些团队小伙伴认为，在提测之前，进行一次大而全的 Review 就可以了。包括对待单元测试的态度也差不多。 关键的点在于，我的个人想法是：如果不能频繁地做 Review ，那么等到最后，可能会迫于交付的压力，对着写了好几个月的代码望洋兴叹，最后就是应付交差，后期改问题的时间也会很长。而小伙伴们似乎对这个悲观预期不是很理解，我也无法说服他们。

结果

在一番讨论后，我们勉强达成了个共识，以模块开发完成为节点进行 Review ，也就是在数月的开发过程中尽量多次 Review ，既不是最后提测统一做一次，也不是每天做。只是这个结果我仍然悲观。

求助

不知道大家有没有遇到这种关于工程实践的分歧争论的情况，有没有什么良好的解决方案呀。自己默默 emo 中

[MiniGhost]

@yzbythesea #12 @smilenceX #18

我对此持相反观点，不知道你听没听过 “代码是用来让人读的，只是顺便让机器执行而已”

CodeReview 不是为了保证 0bug 。比如一些边界值、异常情况没有考虑，直到测试阶段才暴露出来这太正常了。

CodeReview 最要求是更简洁、更已读、更适合的代码，比如是否落实了项目规范中的 MVC 、DDD 、比如是否 3 行代码就可以搞定的事情但是你不知道你写了 30 行、比如是否满足了 SOILD 原则等等。


简而言之：Code Review 是用来保证代码质量，测试是用来保证产品质量，这两者并不是一个东西。

[k9982874]

你们的流程就有问题，按工单提 PR ，按 PR 去 review ，一个 PR 两人以上 approval 就可以合并，人不够的小团队由技术老大把关

[smilenceX]

@MiniGhost
我前面赞同的是“保证代码质量依赖的是各种测试手段“，以及“无效的规章制度影响工作舒适度”
我在打上一个回复是混淆了代码质量和产品质量的概念，可能因此引起的你的误解。

总的来说，我们的看法应该没有冲突。

[smilenceX]

@smilenceX
更正错别字

我前面赞同的是“保证代码质量依赖的是各种测试手段”，以及“无效的规章制度影响工作舒适度”
我在写上一个回复时混淆了代码质量和产品质量的概念，可能因此引起了你的误解。

总的来说，我们的看法应该没有冲突。

[MiniGhost]

@smilenceX #23 [握手]

[libook]

我的团队一开始也是大而全的 review ，但是 review 出问题之后是需要改的，改完后还需要再次 review 。

试行一段时间之后团队成员反馈每次 review 时间很长，需要数小时甚至一两天，比较煎熬，而且大多情况下问题是连锁式的，早期一个问题解决后就不会出现后续一系列的问题。

后来我们就更换为每天一次 review ，每天下午下班前，6 人团队每人不到 10 分钟给大家讲解当天写的代码，然后其他成员一起 review 、提出问题。

[golangLover]

少量你才能看懂，不然大量交上去，一次看两个小时的你们会喜欢看？最后就荒废了

[yzbythesea]

@MiniGhost

我指的“代码质量”里面最重要的环节就是 0 bug （或者你讲的产品质量），也包括你认为的狭义的“代码质量”，比如 coding best pratice ，OOD ，可阅读性等。但实际工作中（个人经验），CR 在处理狭义代码质量上作用不大，因为我觉得比如 OOD 这是基本素质吧，一般只有应届生需要更多提醒下。

[tool2d]

@MiniGhost "CodeReview 最要求是更简洁、更已读、更适合的代码"

世界上没有银弹，没有所谓简洁的代码。

只要项目需求复杂，代码就会随着时间推移，不断变复杂，和熵增原理一样。

定期重构可以缓解这一现象，但这时候 CodeReview 的重要性就下降了。因为你上次 review 的代码，下次很可能直接就被删掉了。

[joesonw]

review 不是批判别人的代码风格，不然量大，大家也闹的不开心。而是找出遗漏的低级错误，或者大家对于这个功能点理解不一致的交流。然后就是交叉熟悉代码，不然负责这块的请个假的时候正好要改不熟悉的话就麻烦大了。

[sampeng]

我弱弱的问一句？？和小伙伴自行沟通？ leader 跑哪去了？
标准是什么有定么？尤其是 java ，一个事情，多个抽象模型都是可行的。标准是什么？

比如
检查重复代码
检查业务逻辑可读性（就是真的看懂了，注释是否合理，满屏幕注释是什么鬼）
检查单元测试覆盖是否合理
review 重要逻辑实现方案是否合理

等等。。要先有标准再有 CR 。盲看？强制把所有人拉在一个水平线是不现实的事

[MiniGhost]

@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 不到点就不写了。

[MiniGhost]

@tool2d #29

第一句话我就不认同，我认为必然有简洁的代码...

举个例子，写个排序，我是自己手写个排序算法简洁，还是直接 array.sort(list) 简洁？


再聊随着需求增加，会不断熵增，这个我完全认同，重构我也能接受。

但是你认为，重构屎山简单，还是重构相对清晰简明的代码简单？

很有可能在熵增的过程中，把控不好，产品层面熵增了 O(n)，代码层面熵增了 O(n²)，Code Review 做得好可以尽可能让业务熵增与代码熵增呈现一个线性关系。

[chocolate518]

其实实际上可能投入测试是最实际的解，单测不是为了发现 bug ，cr 也不会降低 bug 。

[night98]

先问个问题，你们老大呢？

[night98]

补充一下回复，关于 code review ，比较好的做法是完成一个业务功能提交一次，单次 review 尽量保持在 300 行以内，以 Java 为例，使用 mybatis 或者 jpa 对数据表进行建模生成 model 后并添加对应枚举后，提交一次 review ，这次 review 速度一般比较快，基本上都是按团队规范和机器自动生成的，没啥太多 review 的地方，最多检查下是不是建模有问题，一般一两分钟搞定。接着就是常规的功能 review 了，通常建议拆分为业务功能多次提交，以最常见的订单系统为例，会拆分出例如订单分页接口，订单详情接口，订单提交接口，这三个接口分别提交三次 pr+review ，主要目的是保持 review 的单个逻辑连续性，如果单次提交在 1000 行以上，作为 review 评审者，其实相对难度较大，通常还需要 clone 到本地详细查看，成本太高，可能就直接点通过省事了，关于 review 其实从出发角度来看，其实是为了提升整体代码质量，避免代码崩坏，减少后续维护成本。比如团队如果有实际的代码规范的话，reviwe 主要就是关注代码规范，加上潜在 bug 发现，以及一些常规的代码质量改进，例如可以用新特性或者其他更好的方式去实现。关于你提的这个问题其实我觉得也挺有意思的，其实还是那个问题，你们老大呢，工程实践这玩意说实话就是有和没有在短期没什么太大差距，都是长期来看才有价值，否则也没有那么多的人会写垃圾代码了

[secondwtq]

我觉得很重要的一个因素是你们有多少资源。
比如项目很紧，那不 review 也无所谓（最后再 review 跟不 review 也没啥区别了 ...）
比如我们 ddl 不紧，现在是模仿开源项目的模式，每个 commit 都 review ，Code Review 是日常工作内容之一。但是效率就低了。
楼里觉得 Code Review 意义不大的可以思考一下为什么很多知名开源项目都在做 Code Review 。当然这种对于一般企业项目来说有点极端了，大概也是很多开源项目喜欢鸽的原因之一吧。