现实是 Code Review 只会拖慢项目进度,真正的隐患往往是 Review 之后合并进了主分支,而很久之后才被发现 - V2EX
V2EX = way to explore
V2EX 是一个关于分享和探索的地方
现在注册
已注册用户请  登录
请不要在回答技术问题时复制粘贴 AI 生成的内容
zqx
V2EX    程序员

现实是 Code Review 只会拖慢项目进度,真正的隐患往往是 Review 之后合并进了主分支,而很久之后才被发现

  •  
  •   zqx 2019-08-24 17:42:42 +08:00 via Android 6571 次点击
    这是一个创建于 2241 天前的主题,其中的信息可能已经有所发展或是发生改变。
    周五上午我把自己的所有 Bug 修好,提 PR,喊同事 Review (Bitbucket 设置必须有 2 人批准,才能合进发布分支)
    然而今天突然被叫起来修 Bug,我想着昨天都搞完了呀。一看是昨天的 PR 还没合并,赶紧再喊同事 Review,这种形式主义的 Code Review 有毛线作用?
    29 条回复    2019-08-26 13:56:22 +08:00
    zqx
        1
    zqx  
    OP
       2019-08-24 17:49:40 +08:00 via Android
    我在 PR 阶段被提出的最多问题就是: 变量命名之类的问题。我认为这类属于协作效率问题,不应该在某个具体 bugfix 的 PR 阶段提出,而应该是每周或每月开小组技术会议的时候一起 check。真正应该关注的是: diff 中是否改动了无关代码,改动部分的影响范围,当然还有程序逻辑是否正确,会不会产生更多潜在的 Bug。
    zqx
        2
    zqx  
    OP
       2019-08-24 17:56:29 +08:00 via Android
    Code Review 的好处太多了,但那只有硅谷或者国内小部分工程师文化的公司才能享受,大多数不那么工程师文化的大厂最后演变成了: 上级要求下级 Approve PR,下级一般会假装看代码,然后 Approve ;同级之间 Code Review,就看互相关系好不好,你讨厌一个程序员,那就在他的 PR 中挑毛病吧。
    JamesR
        3
    JamesR  
       2019-08-24 17:56:30 +08:00
    只能想办法加快审了。
    我绝大多数 Bug 都是程序跑上 3 个月以上才能暴露,哈哈。
    Jiavwen
        4
    Jiavwen  
       2019-08-24 18:40:34 +08:00
    你每次 PR 是否有足够的测试覆盖?如果没有,那么合并之后出现 bug 也是正常的
    1424659514
        5
    1424659514  
       2019-08-24 18:51:01 +08:00
    zqx
        6
    zqx  
    OP
       2019-08-24 19:27:10 +08:00 via Android
    @Jiavwen 前端,涉及逻辑的部分是有单元测试的,其他 UI 相关的都是人工检查,这部分很难用程序逻辑去描述是否正确
    arrow8899
        7
    arrow8899  
       2019-08-24 19:29:02 +08:00
    只能说明你们 Code Review 没做对,不能说 Code Review 没用; Code Review 是用来改进代码质量和知识分享的,至于代码风格、BUG 等,应该由对应的代码质量检测工具和单元测试来做。
    hoyixi
        8
    hoyixi  
       2019-08-24 19:48:32 +08:00   2
    Code Review 没问题,而是你们自己特色的 Code Review 有问题
    fonlan
        9
    fonlan  
       2019-08-24 21:55:48 +08:00 via Android
    流程不规范
    kaedea
        10
    kaedea  
       2019-08-24 21:59:28 +08:00 via Android   2
    平均研发素质不达标的队伍不适合 Code Review,特别是 bat 里的队伍
    jedihy
        11
    jedihy  
       2019-08-25 03:44:30 +08:00 via iPhone
    代码有 bug 不能怪 CR 的。而且,项目进度问题是你们项目规划本身就做得不好。
    zqx
        12
    zqx  
    OP
       2019-08-25 04:57:33 +08:00 via Android
    各位,我说了 Code Review 是好的,但是多数公司包括大厂也一样,最终会把技术问题演变成流程问题。
    关于流程,Git Flow 工作流,双周迭代(固定隔周的星期二发布窗口),很多项目管理的细节都贴近敏捷软件开发。都是最佳实践啊,哪里出问题了?
    我觉得还是人的问题,平均素质差(无论技术上还是价值观上)的即使用了最佳实践,结果也不一定好
    wd
        13
    wd  
       2019-08-25 06:58:39 +08:00 via iPhone
    一把刀拿来杀猪还是削苹果那当然是你们自己的事情,关刀屁事
    justfortest
        14
    justfortest  
       2019-08-25 08:38:25 +08:00
    Code Review 真的挺难实行的,毕竟不是每个人都对逻辑很清楚,而且很多团队所谓的 Code Review 只是拉几个人开会而已,并不能发现什么问题,作用不大,相比 Code Review 我认为单元测试、集成测试的作用更大。
    zqx
        15
    zqx  
    OP
       2019-08-25 08:44:56 +08:00 via Android
    @wd 在养猪场削苹果和苹果园杀猪,都不合适,大多数公司就是这种环境。
    Antihank
        16
    Antihank  
       2019-08-25 09:35:13 +08:00   1
    不 review 的你难以想象你的同事能写出多么愚蠢的代码,做出多么臃肿的设计。
    razertory
        17
    razertory  
       2019-08-25 10:41:41 +08:00
    Code Review 有时候需要一定程度配合 Unit Test
    seki
        18
    seki  
       2019-08-25 10:58:10 +08:00
    ui 相关的当然可以自动测啊,e2e,image diff 之类的,有总好过没有
    xiubin
        19
    xiubin  
       2019-08-25 11:03:13 +08:00
    当需求都写不完的时候,不计入工时的 CR 都是耍流氓

    最多也就是看看命名或者单独的方法内部逻辑
    GoLand
        20
    GoLand  
       2019-08-25 11:18:15 +08:00
    reviewer 需要有责任心,先理解业务需求是什么,并且尝试理解同事的逻辑,尽量找出逻辑 bug 以及一些代码层面的 bug。反正我 review 同事 PR 的时候,都把需求当成自己的需求,首先我会想一想同事的实现方式有没有问题,如果是我我会怎么实现,是不是比这个实现方式更好,如果更好那么我会提出来;然后我会详细看 diff,不要吝啬发 comment。

    大多数人如果只有自己一个人写代码,没有 review 是很容易出错的,写个 typo 啥的,逻辑进到一个死胡同,是经常的事吧,很多时候自己想半天都没觉得有错,而别人一看就知道有错。
    celeron533
        21
    celeron533  
       2019-08-25 12:55:49 +08:00
    Code revew 内容有好几种:
    1. 看拼写、规范
    比如 typo (还不是那种自动拼写检查能查出来的那种),使用了错误的变量,缩进不合格,注释不详细等
    2. 看业务
    这个真的只有看需求和经验了。明明这个操作要把购物车里所有的东西打折,你却把收藏夹打折
    3. 看代码实现
    “再开个线程,不要阻塞 UI ”
    4. 排雷
    “下个月我离职,所以这段代码在 3 个月后会删库”
    ParadiseDS
        22
    ParadiseDS  
       2019-08-25 13:36:24 +08:00 via Android
    review 配合单测很重要,review 实现的时候,看的基本是大概的框架和流程,功能逻辑的正确性交给单测保障,流程没大问题直接去看单测方案是否全面,所以我个人在单测的要求和 review 单测的精力往往投入更多
    fuyufjh
        23
    fuyufjh  
       2019-08-25 14:18:59 +08:00
    为啥要搞成 2 人批准?个人觉得 1 人足够了
    middleware
        24
    middleware  
       2019-08-25 15:06:01 +08:00
    Code review 是为了保证你的 code 不会违反一些原则(比如 single source of truth, don't repeat yourself )。这样以后发现问题更好处理。而不是保证没有 bug。
    Justin13
        25
    Justin13  
       2019-08-25 18:02:04 +08:00 via Android
    真正的隐患往往是 Review 之后合并进了主分支,而很久之后才被发现

    幸存者偏差
    weixiangzhe
        26
    weixiangzhe  
       2019-08-25 18:03:11 +08:00
    git lab code review 的时候只能看到修改与新增的东西,没有看到完整代码和业务逻辑也很难明白对方是在做什么,而且大家都这么忙,感觉只能看看风格和明显的逻辑错误这种东西了吧;但是 review 一下确实也能学习一下对象的思路啥的
    ZiLong
        27
    ZiLong  
       2019-08-26 12:03:40 +08:00
    @weixiangzhe 羡慕你们能学习对象的思路的
    weixiangzhe
        28
    weixiangzhe  
       2019-08-26 12:39:45 +08:00 via iPhone
    @ZiLong 打错字啦 不搞的
    ZiLong
        29
    ZiLong  
       2019-08-26 13:56:22 +08:00
    @weixiangzhe 不搞的,我们只是 do chicken right!
    关于     帮助文档     自助推广系统     博客     API     FAQ     Solana     5558 人在线   最高记录 6679       Select Language
    创意工作者们的社区
    World is powered by solitude
    VERSION: 3.9.8.5 29ms UTC 07:28 PVG 15:28 LAX 00:28 JFK 03:28
    Do have faith in what you're doing.
    ubao snddm index pchome yahoo rakuten mypaper meadowduck bidyahoo youbao zxmzxm asda bnvcg cvbfg dfscv mmhjk xxddc yybgb zznbn ccubao uaitu acv GXCV ET GDG YH FG BCVB FJFH CBRE CBC GDG ET54 WRWR RWER WREW WRWER RWER SDG EW SF DSFSF fbbs ubao fhd dfg ewr dg df ewwr ewwr et ruyut utut dfg fgd gdfgt etg dfgt dfgd ert4 gd fgg wr 235 wer3 we vsdf sdf gdf ert xcv sdf rwer hfd dfg cvb rwf afb dfh jgh bmn lgh rty gfds cxv xcv xcs vdas fdf fgd cv sdf tert sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf sdf shasha9178 shasha9178 shasha9178 shasha9178 shasha9178 liflif2 liflif2 liflif2 liflif2 liflif2 liblib3 liblib3 liblib3 liblib3 liblib3 zhazha444 zhazha444 zhazha444 zhazha444 zhazha444 dende5 dende denden denden2 denden21 fenfen9 fenf619 fen619 fenfe9 fe619 sdf sdf sdf sdf sdf zhazh90 zhazh0 zhaa50 zha90 zh590 zho zhoz zhozh zhozho zhozho2 lislis lls95 lili95 lils5 liss9 sdf0ty987 sdft876 sdft9876 sdf09876 sd0t9876 sdf0ty98 sdf0976 sdf0ty986 sdf0ty96 sdf0t76 sdf0876 df0ty98 sf0t876 sd0ty76 sdy76 sdf76 sdf0t76 sdf0ty9 sdf0ty98 sdf0ty987 sdf0ty98 sdf6676 sdf876 sd876 sd876 sdf6 sdf6 sdf9876 sdf0t sdf06 sdf0ty9776 sdf0ty9776 sdf0ty76 sdf8876 sdf0t sd6 sdf06 s688876 sd688 sdf86