V2EX = way to explore
V2EX 是一个关于分享和探索的地方
现在注册
已注册用户请  登录
V2EX 提问指南
niceyuri
V2EX  ›  问与答

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

  •  3
     
  •   niceyuri · 2022-03-18 10:42:17 +08:00 · 4141 次点击
    这是一个创建于 979 天前的主题,其中的信息可能已经有所发展或是发生改变。

    背景

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

    情况

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

    结果

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

    求助

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

    37 条回复    2022-03-19 11:57:25 +08:00
    coolzjy
        1
    coolzjy  
       2022-03-18 10:46:18 +08:00   ❤️ 5
    你的想法是理想,小伙伴的想法是现实。
    zzfer
        2
    zzfer  
       2022-03-18 10:53:08 +08:00
    别的不知道,互相 Review 效果挺差的。除非有一套人人都认可的标准且你们所有人都能认真执行。
    liuzhaowei55
        3
    liuzhaowei55  
       2022-03-18 10:53:14 +08:00 via iPhone
    提高单元测试覆盖率吧,cr 在我看来大部份时间只能找出来一些语言层面的问题,对业务上的细节覆盖不到,除非这些业务细节大家都很熟悉。
    maichael
        4
    maichael  
       2022-03-18 11:00:26 +08:00
    在这方面来说没有一步到位的方法,总是先定个框架,然后再慢慢根据真实情况调整。

    Code Reivew 最重要的是定标准,不然的话总会在 Review 过程中产生分歧,导致时间都浪费在这上面。

    另外,说服其实不难; Review 一两百行的更改和 Review 一两万行的更改,你更愿意 Review 哪一个。更不要说后者这种长时间段的改动,往往写代码那个人都会忘记很多其中的细节,Review 的过程中有很多时间都会浪费在这上面。
    niceyuri
        5
    niceyuri  
    OP
       2022-03-18 11:01:07 +08:00
    @coolzjy 可能之前忘了补充前提,我的想法就是我之前的现实
    niceyuri
        6
    niceyuri  
    OP
       2022-03-18 11:01:18 +08:00
    @maichael 好办法!
    niceyuri
        7
    niceyuri  
    OP
       2022-03-18 11:02:43 +08:00
    @zzfer 一针见血。所以我们 Review 是基于公司的编码规范来的,同时也鼓励讨论最佳实践。
    iyaozhen
        8
    iyaozhen  
       2022-03-18 11:03:48 +08:00   ❤️ 1
    个人认为 CR 还是非常有用

    我是 QA ,RD 的代码我都会 CR 。
    我对业务还是相对熟悉(这个前提很重要),CR 不是发现语法问题,这些静态代码检查就行了,主要是发现业务逻辑问题,还有实现的问题,实现不能得过且过,还是能发现很多问题的
    还有个作用就是促进相互了解,两个人开发模块,可能都重复造轮子了,互通一下比较好。还有就是你以后也会开发别人的模块,相互了解团队人员也能相互 backup
    其余还有个好处就是大家写代码会多想一点,把代码写好。我们团队之前就遇到一个模块的代码 review 了一星期还没合进去,有了这次经历后续那个 rd 写的代码质量就好很多了。(当然这个看人,说不定别人还恨你)

    最后频率问题
    「每天提交代码时互相 Review 一下,人也轻松」你这样不太好,代码不是按行或者按天分的,今天写的逻辑都不完整,而且每天提交也不是个好习惯。应该是能运行的一个子功能提交一次( bugfix 除外),基于此 review 。提测前可以再拉上 QA 一起 review 下
    xuboying
        9
    xuboying  
       2022-03-18 11:07:15 +08:00
    OP 提的 Review 是已经去除 LINT 等机器能自动扫描以后,单纯的人工 review 部分么?
    Rwing
        10
    Rwing  
       2022-03-18 11:11:50 +08:00
    尽量把 codereview 放在 pull request 里,也就是每次的 pr 必须要 review 才能 merge
    MiniGhost
        11
    MiniGhost  
       2022-03-18 11:17:40 +08:00
    你的观点是正确的,少量多次肯定是好的

    我之前遇到过一些同事,一口气提一两千行代码,这是指望 Reviewer 是个神仙吗?
    错误的习惯就应该就改正,而不是迁就他

    还有就是,这种事情给我的启发跟教训就是,如果不是很好的团队,就不要搞民主,搞专政搞一言堂。

    先把自己的方案推下去,日常多留心一下大家的执行情况,团队里面人员有执行不到位的即时纠正,之后再看情况了解了解大家的反馈,是否要做一些调整
    yzbythesea
        12
    yzbythesea  
       2022-03-18 11:20:58 +08:00   ❤️ 1
    无法理解你的看法。确保代码质量不应该是依赖单元测试,整合测试,Canary 测试这些客观手段进行吗?第一过度依赖 CR 本身就不靠谱;第二 CR 质量和 CR 大小没有关联;第三无效的规章制度定得太多,会影响人家工作舒适感(就和打卡上班一样)
    niceyuri
        13
    niceyuri  
    OP
       2022-03-18 11:23:08 +08:00
    @xuboying 是的,规范、缺陷、依赖项漏洞都有静态扫描工具
    niceyuri
        14
    niceyuri  
    OP
       2022-03-18 11:24:21 +08:00
    @Rwing 我们用了 Gerrit ,只是说通过的权限放得很松,不想太过限制大家。大家一般就不看代码直接点了
    niceyuri
        15
    niceyuri  
    OP
       2022-03-18 11:25:57 +08:00
    @iyaozhen 感谢~ 拓展了新的视野
    longgediyi999
        16
    longgediyi999  
       2022-03-18 11:35:46 +08:00   ❤️ 2
    让摸鱼的小伙伴无所遁形 一天了 啥也没写
    RiceNoodle
        17
    RiceNoodle  
       2022-03-18 11:45:01 +08:00
    团队一直在做 Code Review ,说一点我觉得好的方式
    1. 功能完整再去做 review ,太零碎的 PR 进行 code review ,很难找到一些逻辑性或者结构性的问题。
    2. 最好有个主程的角色,或者某个业务主程的角色,由他决定是否能够 approve ,允许 merge 。一票否决,这样能够保持某个功能模块,有统一的标准。
    smilenceX
        18
    smilenceX  
       2022-03-18 14:01:02 +08:00 via Android
    @yzbythesea 非常赞同。
    C603H6r18Q1mSP9N
        19
    C603H6r18Q1mSP9N  
       2022-03-18 14:19:42 +08:00
    我们以前是(以前是):每天早上站会,团队成员可以选择 今天 review 或者后面,但是每个功能总会 review 一遍,review 的时候可以看写的有没有问题或者漏洞,有没有更好的写法等,每次 review 都是一步小成长
    niceyuri
        20
    niceyuri  
    OP
       2022-03-18 14:45:36 +08:00
    @shanghai1998 这个办法应该团队小伙伴也能接受~
    MiniGhost
        21
    MiniGhost  
       2022-03-18 14:49:03 +08:00
    @yzbythesea #12 @smilenceX #18

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

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

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


    简而言之:Code Review 是用来保证代码质量,测试是用来保证产品质量,这两者并不是一个东西。
    k9982874
        22
    k9982874  
       2022-03-18 14:57:33 +08:00
    你们的流程就有问题,按工单提 PR ,按 PR 去 review ,一个 PR 两人以上 approval 就可以合并,人不够的小团队由技术老大把关
    smilenceX
        23
    smilenceX  
       2022-03-18 15:22:53 +08:00 via Android
    @MiniGhost
    我前面赞同的是“保证代码质量依赖的是各种测试手段“,以及“无效的规章制度影响工作舒适度”
    我在打上一个回复是混淆了代码质量和产品质量的概念,可能因此引起的你的误解。

    总的来说,我们的看法应该没有冲突。
    smilenceX
        24
    smilenceX  
       2022-03-18 15:25:17 +08:00 via Android
    @smilenceX
    更正错别字

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

    总的来说,我们的看法应该没有冲突。
    MiniGhost
        25
    MiniGhost  
       2022-03-18 15:28:02 +08:00
    @smilenceX #23 [握手]
    libook
        26
    libook  
       2022-03-18 15:34:12 +08:00
    我的团队一开始也是大而全的 review ,但是 review 出问题之后是需要改的,改完后还需要再次 review 。

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

    后来我们就更换为每天一次 review ,每天下午下班前,6 人团队每人不到 10 分钟给大家讲解当天写的代码,然后其他成员一起 review 、提出问题。
    golangLover
        27
    golangLover  
       2022-03-18 15:36:26 +08:00 via Android
    少量你才能看懂,不然大量交上去,一次看两个小时的你们会喜欢看?最后就荒废了
    yzbythesea
        28
    yzbythesea  
       2022-03-18 15:56:11 +08:00
    @MiniGhost

    我指的“代码质量”里面最重要的环节就是 0 bug (或者你讲的产品质量),也包括你认为的狭义的“代码质量”,比如 coding best pratice ,OOD ,可阅读性等。但实际工作中(个人经验),CR 在处理狭义代码质量上作用不大,因为我觉得比如 OOD 这是基本素质吧,一般只有应届生需要更多提醒下。
    tool2d
        29
    tool2d  
       2022-03-18 16:19:57 +08:00
    @MiniGhost "CodeReview 最要求是更简洁、更已读、更适合的代码"

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

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

    定期重构可以缓解这一现象,但这时候 CodeReview 的重要性就下降了。因为你上次 review 的代码,下次很可能直接就被删掉了。
    joesonw
        30
    joesonw  
       2022-03-18 17:14:13 +08:00 via iPhone
    review 不是批判别人的代码风格,不然量大,大家也闹的不开心。而是找出遗漏的低级错误,或者大家对于这个功能点理解不一致的交流。然后就是交叉熟悉代码,不然负责这块的请个假的时候正好要改不熟悉的话就麻烦大了。
    sampeng
        31
    sampeng  
       2022-03-18 17:25:39 +08:00
    我弱弱的问一句??和小伙伴自行沟通? leader 跑哪去了?
    标准是什么有定么?尤其是 java ,一个事情,多个抽象模型都是可行的。标准是什么?

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

    等等。。要先有标准再有 CR 。盲看?强制把所有人拉在一个水平线是不现实的事
    MiniGhost
        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 不到点就不写了。
    MiniGhost
        33
    MiniGhost  
       2022-03-18 17:45:56 +08:00
    @tool2d #29

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

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


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

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

    很有可能在熵增的过程中,把控不好,产品层面熵增了 O(n),代码层面熵增了 O(n²),Code Review 做得好可以尽可能让业务熵增与代码熵增呈现一个线性关系。
    chocolate518
        34
    chocolate518  
       2022-03-18 18:00:23 +08:00
    其实实际上可能投入测试是最实际的解,单测不是为了发现 bug ,cr 也不会降低 bug 。
    night98
        35
    night98  
       2022-03-19 02:25:20 +08:00
    先问个问题,你们老大呢?
    night98
        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 发现,以及一些常规的代码质量改进,例如可以用新特性或者其他更好的方式去实现。关于你提的这个问题其实我觉得也挺有意思的,其实还是那个问题,你们老大呢,工程实践这玩意说实话就是有和没有在短期没什么太大差距,都是长期来看才有价值,否则也没有那么多的人会写垃圾代码了
    secondwtq
        37
    secondwtq  
       2022-03-19 11:57:25 +08:00
    我觉得很重要的一个因素是你们有多少资源。
    比如项目很紧,那不 review 也无所谓(最后再 review 跟不 review 也没啥区别了 ...)
    比如我们 ddl 不紧,现在是模仿开源项目的模式,每个 commit 都 review ,Code Review 是日常工作内容之一。但是效率就低了。
    楼里觉得 Code Review 意义不大的可以思考一下为什么很多知名开源项目都在做 Code Review 。当然这种对于一般企业项目来说有点极端了,大概也是很多开源项目喜欢鸽的原因之一吧。
    关于   ·   帮助文档   ·   博客   ·   API   ·   FAQ   ·   实用小工具   ·   1178 人在线   最高记录 6679   ·     Select Language
    创意工作者们的社区
    World is powered by solitude
    VERSION: 3.9.8.5 · 25ms · UTC 18:21 · PVG 02:21 · LAX 10:21 · JFK 13:21
    Developed with CodeLauncher
    ♥ Do have faith in what you're doing.