V2EX = way to explore
V2EX 是一个关于分享和探索的地方
现在注册
已注册用户请  登录
sockpuppet9527
V2EX  ›  程序员

谈谈 code review?

  •  
  •   sockpuppet9527 · 2020-07-29 09:57:45 +08:00 · 6420 次点击
    这是一个创建于 1338 天前的主题,其中的信息可能已经有所发展或是发生改变。
    • 优点:保证代码质量,保证工程代码风格一致。
    • 缺点:过度 review 导致项目效率低下,遇到不专业的 review 只想喷人。

    最近就遇到个烦人的,改来改去,就写个模块的接口,反反复复改,想到喊我改到哪。 比如一个函数:

    int xxxx_init(CTX * ctx,A *a){
    	if (xxx){
        	return rc;
        }
        xxxx // 实际逻辑代码段
        return rc;
    }
    

    一般来说,项目风格就是这样的,先检查 ctx 啥的,然后如果实际逻辑,最后返回调用状态。 这个方法能给我提 3 个 comments 。

    1. it is simple memcpy... do we really need all the checks below?
    2. i guess this function should return,remove A * a 。
    3. is it documented on API level? (实际逻辑代码段上一顿 bb)

    看到这个我都不想回也不再改了,想问你调用 CTX 是有状态的,你调用函数前不需要 check ?其二本身方法逻辑就是大页来分配 A,名字也叫 xxx_init,我还返回个毛球啊。

    搞得真想跑路。

    35 条回复    2020-07-30 10:47:49 +08:00
    fengjianxinghun
        1
    fengjianxinghun  
       2020-07-29 10:00:54 +08:00   ❤️ 2
    code review 确实 sb 。
    fengjianxinghun
        2
    fengjianxinghun  
       2020-07-29 10:02:01 +08:00   ❤️ 6
    深层问题从来看不出来,只能在一些格式风格上吹毛求疵以符合 code review kpi 。
    Salvation
        3
    Salvation  
       2020-07-29 10:09:28 +08:00
    核心问题不是 cr,而是 cr 的执行方式,执行者。
    shenlanAZ
        4
    shenlanAZ  
       2020-07-29 10:15:55 +08:00
    感觉你们的 code review 少了连带责任,如果 reviewer 的建议导致业务出了问题 reviewer 需要承担一定的责任。

    不过我觉得 code review 最大的好处就是减少项目的烂代码,有些实习生或者代码写的烂的人 ,提交上去的代码能让你粗口半小时,code review 在解决问题的同时 还能帮助他怎么才能做到更好。
    MarshallMathers
        5
    MarshallMathers  
       2020-07-29 10:18:02 +08:00
    哪一家啊 lz??
    coderluan
        6
    coderluan  
       2020-07-29 10:18:34 +08:00
    你们这是随时聊天或者发邮件 review 的? 建议还是开会 review 吧, 做好记录, 效率提升很明显的, 起码不会总出现一样问题浪费时间.
    luckyrayyy
        7
    luckyrayyy  
       2020-07-29 10:18:35 +08:00
    code review 不能背这个锅吧,要喷也应该喷同事 xxx
    sockpuppet9527
        8
    sockpuppet9527  
    OP
       2020-07-29 10:27:45 +08:00
    @Salvation #3 非 maintainer 的 review,往往让人很迷惑
    @coderluan #6 有专门的 review system
    sockpuppet9527
        9
    sockpuppet9527  
    OP
       2020-07-29 10:29:11 +08:00
    @luckyrayyy #7 差不多是这个意思。缺点特指:过度 review,咬文嚼字。
    securityCoding
        10
    securityCoding  
       2020-07-29 10:31:23 +08:00
    code review 需要很深的功力 ,需要从代码和业务视角来审视代码
    GoLand
        11
    GoLand  
       2020-07-29 10:31:48 +08:00
    你这个是 reviewer 的问题,典型的爱钻牛角尖,不会 review 来 review 代码简直比重写一百遍还难受,建议以后你 review 的时候,给他也来一下类似的操作,每块逻辑提几个 commet,他就知道有多难受了,后面应该就会改进。
    wutiantong
        12
    wutiantong  
       2020-07-29 10:35:34 +08:00
    怼呀,所以是个人都是 review 另一个人的代码么?肯定不是啊,你凭啥来 review 我的代码,你水平够么?
    就这 3 点 bb,你来改,我看看你要改成啥样子。
    coderluan
        13
    coderluan  
       2020-07-29 10:35:38 +08:00
    @sockpuppet9527 系统不能取代开会的, 比如这种再开会的时候当着大家的面提出来, 之后对方肯定会收敛不少, 当然你也可以在项目会议上说一下, 你自己憋着和网友吐槽都没啥用, 你都自己想跑了, 还有啥顾忌, 开会当场撕他啊.
    wutiantong
        14
    wutiantong  
       2020-07-29 10:36:10 +08:00
    @wutiantong 都是==》都能
    sockpuppet9527
        15
    sockpuppet9527  
    OP
       2020-07-29 10:52:15 +08:00
    @coderluan #13
    就拿第二点来说,假如需要你把方法名改成 xxxxx_create,然后把参数中的指针改为返回,这点我就很难反驳他,因为这两种方式是一样的(在本项目中)。这种事情你怎么和他扯的清楚?
    我觉得开会讲这种问题,没个头,我目前来说,只能妥协。之后这块谁爱改谁改。约等于掉了一次坑。
    sockpuppet9527
        16
    sockpuppet9527  
    OP
       2020-07-29 10:53:53 +08:00   ❤️ 1
    @wutiantong #12 是这样的,我个人始终认为应该就几个 maintainer 来 review 就好了,现在互相 review (而且国家还不同)搞得效率极低。
    gadsavesme
        17
    gadsavesme  
       2020-07-29 10:57:50 +08:00
    我们之前都是每周挑个一两小时开会组里人一起 review,规范大家定,但是得遵守。
    coderluan
        18
    coderluan  
       2020-07-29 11:07:17 +08:00
    @sockpuppet9527 开会不是让你讲具体问题, 而且去约定 review 的范围, 没实际影响的内容就别提, 由 maintainer 自己定, 但是你说你们国家不同, 这个可能才是说这个的障碍, 反正你说这个我第一时间就感觉对方是印度人, 算是个人歧视吧, 但是我的印度同事确实也这样, 也确实没办法说.
    xuanbg
        19
    xuanbg  
       2020-07-29 11:19:30 +08:00
    代码评审不必责备求全,可以逐步推进。
    第一步要解决的是代码风格问题,统一的代码风格评审起来才能事半功倍。
    第二步是代码规范问题,Java 的话按阿里的规约取舍一下就好。规范执行到位,bug 能少一大半。
    第三步是代码结构问题,编程最大的问题不是写错了代码,而是代码没写对地方。这个问题包括但不限于:代码冗余、过大的类和方法、方法有太多的参数、项目结构混乱或根本没有结构……说白了就是没有任何封装或者错误的封装。

    以上做好了,代码基本也就没啥毛病了
    hakono
        20
    hakono  
       2020-07-29 11:37:08 +08:00   ❤️ 1
    一个 21 岁入职的年轻人做 code review (有前职经验),进来就对着我的代码给了十来条 comment,然后我觉得有意义的地方改了,没问题的地方没改,回复了一下为什么不改。然后对方直接开始了和我你来我往几千字以上的文字交锋

    我因为日语非母语,对方是日本人,还一大段日语打下来从不加标点(就连日本人同事都只喊他这日语受不了),和他互相交锋浪费了 2 天时间,最后代码没合并,项目没进展他还不依不饶,把项目开始至今为止的经纬和为什么这么做都给他解释了一遍,怎么解释都不听。
    最后气得我直接爆粗,结果对方还跑去组长那让组长做定夺。自然了解项目始末的组长认为我的写法比较好,最后那人才罢休。感觉这两天时间是彻底浪费掉了

    有些人做 code review 是真的纯属浪费时间
    otakustay
        21
    otakustay  
       2020-07-29 11:53:56 +08:00
    风格一致不应该是你靠人去 Review 的
    Sasasu
        22
    Sasasu  
       2020-07-29 12:05:48 +08:00
    明显指针作为入参比在函数内部 new 一个作为返回更好。

    把对象生命周期控制交给调用者有更好的性能和更灵活的使用方式
    Leigg
        23
    Leigg  
       2020-07-29 12:26:41 +08:00 via Android
    @gadsavesme [大家一起] 实际上是很难的,只有非常扁平化的团队。
    netnr
        24
    netnr  
       2020-07-29 12:37:54 +08:00
    安排不对口的人做专业的事
    bsg1992
        25
    bsg1992  
       2020-07-29 13:04:26 +08:00
    cr 是一件非常吃力不讨好的事情。首先 review 的人必须要懂你负责的业务,如果不懂业务背景单纯的只看代码的 review 是没有任何意义的,反而会出现负面效果。
    代码风格和结构可以通过静态检查来约束。
    团队人员多起来后 cr 是一个非常难以维持的一件事情。
    我觉得 cr 最佳实践 符合小团队开发模式
    iyaozhen
        26
    iyaozhen  
       2020-07-29 13:33:14 +08:00   ❤️ 2
    所以这就是 CR 推不起来的原因

    CR 有这有那的问题,并不是 CR 本身有问题,还是做的不够好的原因。
    1. CR 需要一个高 level 的来最终决定、平级可以给 review 意见
    2. 大量代码量 CR 可以组织会议的方式进行
    3. 不已评论数为唯一 KPI (可以作为大盘参考)

    但这个事情其实需要团队内部文化认同,比如 CR 执行初期我们就遇到过有个同学 2 周硬是没有合入代码,但经过一段时间磨合就好额。
    最后珍惜还有 CR 的公司吧
    wangyzj
        27
    wangyzj  
       2020-07-29 13:39:24 +08:00
    如网友想象的那么闲的公司 code review 估计也就是看格式,命名,和一些基础的样式的东西,深层次的东西不会有人有那么多时间去看
    只有网友想象不到的闲的公司才会把逻辑,算法啥的看到底,当然效率肯定也是低下的
    大部分 code review 估计都没有,测试过了就拉倒
    tinkgoose
        28
    tinkgoose  
       2020-07-29 13:47:14 +08:00
    kop1989
        29
    kop1989  
       2020-07-29 13:51:22 +08:00
    code review 没错,但是 code review 的成本是极其高昂的。
    如果是严谨的 code review,那么基本上审核者就要在脑中把程序编一遍才行。也就是说代码审核和编码其实就差一个“把代码打出来”的工序。

    所以基本上国内 code review 都成了“格式审核”。
    tinkgoose
        30
    tinkgoose  
       2020-07-29 13:53:28 +08:00
    @wangyzj 手滑了囧,不过一般来说格式、命名简单的我这边都是依靠工具而不是人来约束,因为确实没那么闲。

    然后不了解这块业务的人去 review 的话,基本只能保证不出大问题,让项目更规整一些。所以这对 reviewer 的要去蛮高的,真的更依赖于 reviewer 的自觉。按经验我被 review 到的时候,都是一些不痛不痒的问题,很少能指出很实质性的问题,往往都是出了问题或者有同事来修改这一块的时候才发现的。

    review 还是蛮有意义的,也不需要一直 review,一般磨合一段时间,就不需要过分地 review 了。
    wangyzj
        31
    wangyzj  
       2020-07-29 14:21:18 +08:00
    @tinkgoose #30 工具,lint 之类的做限制肯定是能解决一部分问题
    核心问题还得看人
    至于人能做到什么程度真的就按照你说的,得看个人水平和自觉性
    所以很可能就会是你遇到的那种不痛不痒的问题

    review 意义不用多说,不过自然最根本的也得看公司的业务发展等多个因素了
    shenqi
        32
    shenqi  
       2020-07-29 16:30:44 +08:00   ❤️ 2
    不要把 code review 当成是挑别人毛病的事情,而是把 code review 当成自己学习成长以及促进别人成长的事情。
    wangritian
        33
    wangritian  
       2020-07-29 17:01:54 +08:00
    code review 和做架构一样吧,需要根据实际情况柔性处理
    jackindata
        34
    jackindata  
       2020-07-29 17:46:53 +08:00
    这个 reviewer 需要接受培训。
    我觉得《代码大全》已经把 code review 讲得很清楚了。
    leekafai
        35
    leekafai  
       2020-07-30 10:47:49 +08:00
    保证代码质量这个就很虚了,你的运行性能跟业务场景是挂钩的,大部分的“保证代码质量”可能就是人肉 linter,因为 viewer 不一定熟知你的业务场景跟需求,除非他有参与到你的项目中,哪怕做个技术观众也好过啥都不知道。
    代码是写给人看的,code review 现在很多也是卡在了人肉 linter 这个层面,像业务逻辑这种,有时候越纠结越歪 B,到最后连需求是啥都混乱了。
    最好的还是写测试,测试过了就是过了,因为测试就是可以跟着业务走的。fuzzer 跑几遍没什么问题还能咋地。
    如果还有要紧地拓展性要考虑,那事实上 code review 也不能完全覆盖掉,否则哪有这么多老系统要重构呢。
    关于   ·   帮助文档   ·   博客   ·   API   ·   FAQ   ·   我们的愿景   ·   实用小工具   ·   3092 人在线   最高记录 6543   ·     Select Language
    创意工作者们的社区
    World is powered by solitude
    VERSION: 3.9.8.5 · 26ms · UTC 14:35 · PVG 22:35 · LAX 07:35 · JFK 10:35
    Developed with CodeLauncher
    ♥ Do have faith in what you're doing.