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

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

  •  3
     
  •   niceyuri · 107 天前 · 3203 次点击
    这是一个创建于 107 天前的主题,其中的信息可能已经有所发展或是发生改变。

    背景

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

    情况

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

    结果

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

    求助

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

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

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

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

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

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

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

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

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

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

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

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


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

    总的来说,我们的看法应该没有冲突。
    smilenceX
        24
    smilenceX  
       107 天前 via Android
    @smilenceX
    更正错别字

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

    总的来说,我们的看法应该没有冲突。
    MiniGhost
        25
    MiniGhost  
       107 天前
    @smilenceX #23 [握手]
    libook
        26
    libook  
       107 天前
    我的团队一开始也是大而全的 review ,但是 review 出问题之后是需要改的,改完后还需要再次 review 。

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

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

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

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

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

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

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

    等等。。要先有标准再有 CR 。盲看?强制把所有人拉在一个水平线是不现实的事
    MiniGhost
        32
    MiniGhost  
       107 天前
    @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  
       107 天前
    @tool2d #29

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

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


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

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

    很有可能在熵增的过程中,把控不好,产品层面熵增了 O(n),代码层面熵增了 O(n²),Code Review 做得好可以尽可能让业务熵增与代码熵增呈现一个线性关系。
    chocolate518
        34
    chocolate518  
       107 天前
    其实实际上可能投入测试是最实际的解,单测不是为了发现 bug ,cr 也不会降低 bug 。
    night98
        35
    night98  
       106 天前
    先问个问题,你们老大呢?
    night98
        36
    night98  
       106 天前
    补充一下回复,关于 code review ,比较好的做法是完成一个业务功能提交一次,单次 review 尽量保持在 300 行以内,以 Java 为例,使用 mybatis 或者 jpa 对数据表进行建模生成 model 后并添加对应枚举后,提交一次 review ,这次 review 速度一般比较快,基本上都是按团队规范和机器自动生成的,没啥太多 review 的地方,最多检查下是不是建模有问题,一般一两分钟搞定。接着就是常规的功能 review 了,通常建议拆分为业务功能多次提交,以最常见的订单系统为例,会拆分出例如订单分页接口,订单详情接口,订单提交接口,这三个接口分别提交三次 pr+review ,主要目的是保持 review 的单个逻辑连续性,如果单次提交在 1000 行以上,作为 review 评审者,其实相对难度较大,通常还需要 clone 到本地详细查看,成本太高,可能就直接点通过省事了,关于 review 其实从出发角度来看,其实是为了提升整体代码质量,避免代码崩坏,减少后续维护成本。比如团队如果有实际的代码规范的话,reviwe 主要就是关注代码规范,加上潜在 bug 发现,以及一些常规的代码质量改进,例如可以用新特性或者其他更好的方式去实现。关于你提的这个问题其实我觉得也挺有意思的,其实还是那个问题,你们老大呢,工程实践这玩意说实话就是有和没有在短期没什么太大差距,都是长期来看才有价值,否则也没有那么多的人会写垃圾代码了
    secondwtq
        37
    secondwtq  
       106 天前
    我觉得很重要的一个因素是你们有多少资源。
    比如项目很紧,那不 review 也无所谓(最后再 review 跟不 review 也没啥区别了 ...)
    比如我们 ddl 不紧,现在是模仿开源项目的模式,每个 commit 都 review ,Code Review 是日常工作内容之一。但是效率就低了。
    楼里觉得 Code Review 意义不大的可以思考一下为什么很多知名开源项目都在做 Code Review 。当然这种对于一般企业项目来说有点极端了,大概也是很多开源项目喜欢鸽的原因之一吧。
    关于   ·   帮助文档   ·   API   ·   FAQ   ·   我们的愿景   ·   广告投放   ·   感谢   ·   实用小工具   ·   2550 人在线   最高记录 5497   ·     Select Language
    创意工作者们的社区
    World is powered by solitude
    VERSION: 3.9.8.5 · 23ms · UTC 11:12 · PVG 19:12 · LAX 04:12 · JFK 07:12
    Developed with CodeLauncher
    ♥ Do have faith in what you're doing.