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

大家所在的公司都做 CodeReview 么?

  •  
  •   peterswan · 2018-08-14 11:05:43 +08:00 · 10299 次点击
    这是一个创建于 2053 天前的主题,其中的信息可能已经有所发展或是发生改变。

    我现在所经历的公司,还没有做 CodeReview 的,也许公司太小的原因,但是我觉得 CodeReview 是非常有效的方式,能提高代码可控性和质量,还能促进技术交流。不知道你们经历的公司有 CodeReview 么,有的话可以分享一下感受,和如何进行 CodeReview 比较好。

    101 条回复    2018-08-19 12:33:36 +08:00
    1  2  
    anonymous0user
        1
    anonymous0user  
       2018-08-14 11:10:17 +08:00
    小公司比大公司更容易 CodeReview ……
    peterswan
        2
    peterswan  
    OP
       2018-08-14 11:11:54 +08:00
    @anonymous0user 为啥我经历的小公司也没有 CodeReview,感觉有的公司是因为项目赶,有的根本就没有这个意识...
    liprais
        3
    liprais  
       2018-08-14 11:12:16 +08:00 via iPhone   ❤️ 1
    做数据库的,不做 code review 就是找死
    peterswan
        4
    peterswan  
    OP
       2018-08-14 11:17:06 +08:00
    @liprais 数据库应该做 CodeReview 是必须的了,这个弄不好就从开发到跑路了。
    NCry
        5
    NCry  
       2018-08-14 11:29:47 +08:00
    我们公司就两个开发。。。
    peterswan
        6
    peterswan  
    OP
       2018-08-14 11:39:24 +08:00
    @NCry。。。那你们俩启不是公司的核心技术了
    NCry
        7
    NCry  
       2018-08-14 11:42:16 +08:00
    @peterswan 公司业务比较简单,对于技术要求不高。实际上我感觉我在慢性自杀,但是又找不到方法跳出来,目前只能呆着。
    yuanrenxue
        8
    yuanrenxue  
       2018-08-14 11:42:34 +08:00
    @NCry 一个写代码,一个补漏 哇咔咔
    zclHIT
        9
    zclHIT  
       2018-08-14 11:42:57 +08:00
    做啊,每个版本快结束的时候改 pmd, checkstyle, codedex, simian 改到想吐,不过代码冲刺的时候 CodeReviewer 就直接点 accept,从来不看。。。
    enenaaa
        10
    enenaaa  
       2018-08-14 11:45:38 +08:00
    现在的这家不但做交叉 review,入库还要审批。 重构优化代码有时候还找不到理由发申请。
    xiaket
        11
    xiaket  
       2018-08-14 11:46:00 +08:00
    找人过 PR 好烦... - .-
    kaito
        12
    kaito  
       2018-08-14 11:47:42 +08:00
    在公司推行 Code Review 的话需要看看周围同事是不是有那种意愿

    如果是平级推进的话,看看同事的技术和代码风格是否 ok,可以的话找他讨论一下有没有意愿,有些程序员很无所谓的,觉得写「好代码」太浪费时间了,review 就是 挑刺的过程,自尊心太强的会非常抵触。

    推行这些的前提需要比较好的同事关系和工作素质,如果都不错的话可以尝试推进一下
    peterswan
        13
    peterswan  
    OP
       2018-08-14 11:48:24 +08:00
    @zclHIT 我觉得不错,对于代码质量和自己习惯都有提高,还可以对于优化进行讨论,不过代码冲刺的时候我觉得可以预留这部分时间,太急的情况只能例外了。
    hasbug
        14
    hasbug  
       2018-08-14 11:48:54 +08:00
    上一家我去的时候给我说会,我害怕我的面条式代码会有危险,待了一段,没有的事。。。
    peterswan
        15
    peterswan  
    OP
       2018-08-14 11:51:03 +08:00
    @kaito 恩恩,我觉得可以和同事商量一下,对于一些代码风格问题可以统一定一个规范,Review 过程也是提高的过程。
    peterswan
        16
    peterswan  
    OP
       2018-08-14 11:52:35 +08:00
    @xiaket 等待 pr 的心理就像等待女朋友消息的心理,哈哈。
    peterswan
        17
    peterswan  
    OP
       2018-08-14 11:53:57 +08:00
    @enenaaa 我觉得领导要是有技术热情,应该会喜欢优化的,重构要看付出的代价了。
    peterswan
        18
    peterswan  
    OP
       2018-08-14 11:55:39 +08:00
    @hasbug 为啥,难道审批的人不在乎都给过。。。
    crayygy
        19
    crayygy  
       2018-08-14 11:57:26 +08:00 via iPhone
    我们 team 里面是必须过的,而且至少有两个人,一个是需要是本模块的,另一个可以是其他的,而且 arch manager 都会查最近的 commit 有没有按照规定来完成
    mikuazusa
        20
    mikuazusa  
       2018-08-14 12:02:10 +08:00
    持续集成流程强制必须过 CR
    winterfell30
        21
    winterfell30  
       2018-08-14 12:02:41 +08:00 via Android
    提代码的时候必须要有该模块的负责人打分,但是感觉大家也不怎么看随手就给过了
    zclHIT
        22
    zclHIT  
       2018-08-14 12:02:43 +08:00 via iPhone
    @peterswan 每次都能把一个月的需求压缩到一周然后搞冲刺,扯皮 2 周,冲刺 1 周,测 1 周
    peterswan
        23
    peterswan  
    OP
       2018-08-14 12:12:54 +08:00
    @crayygy 你们这个流程可以的,避免了哪些乱七八糟的提交和代码。
    poorcai
        24
    poorcai  
       2018-08-14 12:37:23 +08:00 via iPhone
    我们部门每周一次
    klren0312
        25
    klren0312  
       2018-08-14 12:43:56 +08:00 via Android
    讲真的,做外包本来就很敢,恨不得早点写完,没人愿意看代码
    d18
        26
    d18  
       2018-08-14 13:01:10 +08:00
    大公司也有代码质量很差的,只要最后能跑起来就行,比如鹅厂。
    wobushizhangsan
        27
    wobushizhangsan  
       2018-08-14 13:03:12 +08:00 via Android
    今年做了个项目别说 review,连测试都没了。开发,上线,生产问题,补丁。
    wuzhizhan
        28
    wuzhizhan  
       2018-08-14 13:07:53 +08:00
    今天需求,明天上线
    dangluren
        29
    dangluren  
       2018-08-14 13:14:08 +08:00
    哇,和楼主很像,目前的公司也不做,之前经历过的都做,虽然有时候有的自尊会受到打击,但感觉是非常有效的方式,由于不做,目前的项目是天天出问题。向领导反馈过,领导貌似自己都不想做
    peterswan
        30
    peterswan  
    OP
       2018-08-14 13:49:55 +08:00
    @dangluren 恩恩 这个就像 12 楼所说的,要大部分人都有这个意识,尤其是技术领导或者团队领导,否则会趋于形式化起不到作用,现在我们也没有这个意识,大多数人更在乎速度,对于质量都想着以后优化,但是以后永远在以后。
    peterswan
        31
    peterswan  
    OP
       2018-08-14 13:51:38 +08:00
    @d18。。。没去过大公司还,不过我觉得大公司推行这个应该更容易吧,资源充足,技术大牛也多,不过这个还是要看领导了
    monkeylyf
        32
    monkeylyf  
       2018-08-14 13:59:07 +08:00
    长远来看不做 code review 就是自杀行为。
    code review
    monkeylyf
        33
    monkeylyf  
       2018-08-14 14:01:26 +08:00
    从中长远来没有 code review 看就是自杀行为。
    了解同事的技术水平,了解同事在做什么,肉眼抓 bug,等等等。 因为要急着上线不审代码的先不说技术水平如何, 工程水平肯定有提高空间。
    HuHui
        34
    HuHui  
       2018-08-14 14:31:19 +08:00
    回武汉后就没做过了。
    hiluluke
        35
    hiluluke  
       2018-08-14 14:49:32 +08:00
    先发 issue,然后技术讨论,然后再发 pr。pr 再 review 一遍。PR 过大,需要拆分。。。
    imdupeng
        36
    imdupeng  
       2018-08-14 15:10:47 +08:00
    没有 codereview,不过老大总喜欢改别人的代码,每次他的意见都很中肯,但是你改了要调试好啊,每次改了出问题,还得我去调试好。
    一方面催进度,一方面跑来改我代码找麻烦。。能不能等逻辑跑完了再来优化呀?搞得我反复在原来的代码上做工。
    mhtt
        37
    mhtt  
       2018-08-14 15:13:24 +08:00
    其实对没有足够的 code review 经验,而去做 code review 是件挺难受的事情
    coolhubery
        38
    coolhubery  
       2018-08-14 15:17:03 +08:00   ❤️ 4
    我部门是做数据库执行引擎的,C++ 14。CodeReview 到了“变态”的地步。。。
    所有 change 都必须尽量的小,必须写 Unit Test,代码覆盖率 100%。一个 change 从开始 push 到最终 merge 最少怎么着也得 5 到 6 轮,当然国外的大牛同事基本上 2/3 个 patch 就 merge 了。
    从错别字,注释的准确性,英语表达是否合适,代码可重用,设计角度是否合适,性能是否有影响,Unit Test 是否真得测到了该有的功能,是否应用了 C++ 14 的标准等诸多方面进行考量。。。
    这样做的好处是作为非常核心的一个组件,模块非常稳定,新增代码量很大,bug 却非常少。
    虽然作为个人来讲,初期自己的修改进展很慢,要被 challenge 很多次,但是一旦熟悉了也就很快了。
    不管是那个公司,核心的组件必然要经过极其严格的 Review,否则后期的成本会非常高。
    nicevar
        39
    nicevar  
       2018-08-14 15:17:49 +08:00
    配置 SonarQube,减少 code review 工作量,同是提升代码质量,还能让人的强迫症变得更严重
    Just1n
        40
    Just1n  
       2018-08-14 15:18:01 +08:00
    我们每个 checkin 都必须有 code review。
    而且,如果修改了不属于自己组的代码,会自动触发一个流程让其他组的人介入进行 review。
    peterswan
        41
    peterswan  
    OP
       2018-08-14 15:28:22 +08:00
    @monkeylyf 同意啊,见了一些项目因为技术债实在开发不下去的项目。。。
    czk1997
        42
    czk1997  
       2018-08-14 15:29:37 +08:00
    能跑就行,出 Bug 再说……测试全靠脸(能跑就行),服务器部署全随缘..
    peterswan
        43
    peterswan  
    OP
       2018-08-14 15:29:48 +08:00
    @hiluluke 这个工作流程有点像 github,你们用的啥平台,github ?? 我也想用这个流程试试
    peterswan
        44
    peterswan  
    OP
       2018-08-14 15:31:46 +08:00
    @imdupeng 下次不要让他直接该代码,让他提 pr 或者开个分支,做好了之后你来 Review,有问题让他改好了在合并,不过这好像是你的老大。。。
    peterswan
        45
    peterswan  
    OP
       2018-08-14 15:35:11 +08:00
    @coolhubery 这个流程有很多值得学习的地方啊,能提供这样严格的 Review 肯定公司也特别棒了,谢谢分享
    lsyAndroid
        46
    lsyAndroid  
       2018-08-14 15:36:32 +08:00 via Android
    没有,全靠自觉,本身就是赶项目的,没有时间搞
    peterswan
        47
    peterswan  
    OP
       2018-08-14 15:37:46 +08:00
    @nicevar 用自动化工具是会加快这个过程,不过我感觉每次 pr 应该粒度尽可能的小,Code Review 压力就不会那么大了,而且可控性更高,如果太大的 pr,直接让他拆分后再 Review。
    peterswan
        48
    peterswan  
    OP
       2018-08-14 15:40:46 +08:00
    @Just1n 恩恩 强制的 Review 才能保证代码可控,不过我觉得为什么要让其他组的人来 review 呢,毕竟其他组的肯定不熟悉业务,找同组的人会不会更节省时间。
    66beta
        49
    66beta  
       2018-08-14 15:45:44 +08:00 via Android
    本组的人,没人负责一个模块,也只能 review 语言本身了,无法察觉业务漏洞
    peterswan
        50
    peterswan  
    OP
       2018-08-14 15:46:01 +08:00
    @czk1997
    @IsyAndroid
    我觉得这样做会加大技术债,现在赶出来的时间以后会加倍付出对于项目来说。当然要看老大有没有这个意识。。。
    A555
        51
    A555  
       2018-08-14 15:46:42 +08:00
    形式大于内容
    peterswan
        52
    peterswan  
    OP
       2018-08-14 15:47:29 +08:00
    @mhtt 刚开始要是没有经验,可以大家一起开个会学习一下 Review 嘛,以后应用到项目中就会越来越熟练了。
    peterswan
        53
    peterswan  
    OP
       2018-08-14 15:49:50 +08:00
    @66beta 对于语言和代码优化相关的 Review 也会提升代码质量,更关键能在 Review 过程中提升自己,既可以挑别人代码的不足也可以学习别人代码的优点。
    hiluluke
        54
    hiluluke  
       2018-08-14 15:50:02 +08:00
    @peterswan 就是用的 github 企业版。
    natscat
        55
    natscat  
       2018-08-14 15:54:08 +08:00
    做啊 代码功能 实现方式 都做
    monkeylyf
        56
    monkeylyf  
       2018-08-14 16:03:48 +08:00
    @peterswan 我亲身经历过没有代码审核,快糙猛急着上线,等代码库到一定体量一个没更新一个版本都是提心吊胆,开发人员身心俱疲,最后项目烂了,公司黄了。
    话说回来,我现在所待的地方,是有代码审核的,同样老板喜欢催上线,大佬们都相互给个 LGTM 都不仔细看。现在代码体量已经很大了,基本两天一个中性 bug 爆炸,一周一个大 bug 核爆。
    流程是用来约束自觉性的,但是完全没有自觉性,有再多流程也没用。
    loveCoding
        57
    loveCoding  
       2018-08-14 18:11:56 +08:00   ❤️ 1
    组内在使用 gerrit 做强制 code review 还算比较好用
    peterswan
        58
    peterswan  
    OP
       2018-08-14 18:33:17 +08:00
    @monkeylyf 恩恩,这个要看有没有一个厉害的技术管理去领导做这个 Review 事情,对 Review 质量进行把关,如果只是应付形式简直就是浪费时间了,还有就是对于 Code Review 可以有一定的奖励制度调用积极性。
    RorschachZZZ
        59
    RorschachZZZ  
       2018-08-14 18:44:10 +08:00
    @monkeylyf 中等或者严重的 bug,你们那不复盘下、讨论下吗
    zhaoxinz
        60
    zhaoxinz  
       2018-08-14 19:31:48 +08:00
    在就职过的公司中,只有印象笔记有融入在日常工作中的标准流程 Codereview,在 commit 之后,通过命令行指定一个人为你 Codereview
    hoichallenger
        61
    hoichallenger  
       2018-08-14 20:38:37 +08:00
    我们公司有 Code Review, 不过正打算用 Pair Programming + Mob Programming 取代 Code Review
    monkeylyf
        62
    monkeylyf  
       2018-08-14 20:48:39 +08:00
    @RorschachZZZ 讨论的。但都是基于表面或者 bug 本身的讨论,深层次原因应该大家都知道但是不会拿出来说罢了。
    xufeng
        63
    xufeng  
       2018-08-14 20:54:46 +08:00
    @NCry 简单的事情可以创新做,《并非喊口号》
    peterswan
        64
    peterswan  
    OP
       2018-08-14 21:13:10 +08:00 via Android
    @hoichallenger 刚刚了解了 Pair Programming 和 Mob Programming,真是个不错的想法,公司的技术管理也很不错啊。
    peterswan
        65
    peterswan  
    OP
       2018-08-14 21:17:45 +08:00 via Android
    @loveCoding 准备搭建一发推荐给团队用。。。
    agagega
        66
    agagega  
       2018-08-14 21:18:10 +08:00 via iPhone
    你们在 review 时有些特别特别小的改动也要新增提交吗?有时候想 commit amend 然后 force push,但是又觉得不太好。看来还是 git 太自由了,用 hg 就没有这种强迫症。
    fanqianger
        67
    fanqianger  
       2018-08-14 21:23:00 +08:00
    @coolhubery 你们什么公司。
    peterswan
        68
    peterswan  
    OP
       2018-08-14 21:36:32 +08:00 via Android
    @agagega 应该是按照功能细分粒度的,我觉得尽可能小的好。还有远程提交信息不是不应该修改嘛,除非只有自己提交分支。而且我觉得应该过了 Review 再入库。
    nanyang24
        69
    nanyang24  
       2018-08-14 21:48:22 +08:00
    很幸运的是,所在的公司技术 leader 强制了这项规定,合并代码前必须经过任意两个同事的 CodeReview。
    我觉得好处是项目代码健壮性得到了一定的保证,对个人的话是学习认识不同风格和 level 的代码,还能熟悉别人做的需求,对整体项目理解有帮助。
    qiaobeier
        70
    qiaobeier  
       2018-08-14 22:00:45 +08:00
    @coolhubery sap 核心部门的啊,运气真不错,我前同事分去做业务据说非常废人。。。
    agagega
        71
    agagega  
       2018-08-14 22:10:14 +08:00 via iPhone
    @peterswan 嗯,我说的就是自己的 PR 里面
    coolhubery
        72
    coolhubery  
       2018-08-14 23:49:32 +08:00
    @fanqianger SAP...
    coolhubery
        73
    coolhubery  
       2018-08-14 23:50:46 +08:00
    @qiaobeier 不了解产品,SAP 的数据库平台技术还是相当不错的。
    ymj123
        74
    ymj123  
       2018-08-15 00:39:59 +08:00 via Android
    @zclHIT findbugs 不跑?
    fanqianger
        75
    fanqianger  
       2018-08-15 04:33:44 +08:00
    @coolhubery 这厉害的啊,hana 挺牛逼的。你是在德国吗
    liyuhang
        76
    liyuhang  
       2018-08-15 07:02:56 +08:00
    我觉得应该先 Review 一下你的标题
    jiangjz
        77
    jiangjz  
       2018-08-15 07:51:25 +08:00 via Android
    不做,所以有的代码维护起来让人想离职😂😂
    jaaazzz
        78
    jaaazzz  
       2018-08-15 08:51:42 +08:00
    中等规模公司,做个屁
    micean
        79
    micean  
       2018-08-15 08:53:47 +08:00
    作为管理人员想知道你们有多少时间花在 code review 上,我每天都忙得要死
    现在只能了解下面人的代码水平,交流一些好的习惯,从编码规范上要求他们自觉
    dosmlp
        80
    dosmlp  
       2018-08-15 08:55:52 +08:00
    没。。。。。。时。。。。。。间。。。。。。
    lxerxa
        81
    lxerxa  
       2018-08-15 08:57:42 +08:00
    团队小的时候还做,后来慢慢放弃了。。。
    wuhanchu
        82
    wuhanchu  
       2018-08-15 09:00:39 +08:00 via Android
    有 review 都是好公司啊
    hasbug
        83
    hasbug  
       2018-08-15 09:06:06 +08:00
    @peterswan 因为项目已经不受项目经理控制,一个老员工总把自己当回事,什么都他说了算,项目经理心累,直接不管了
    peterswan
        84
    peterswan  
    OP
       2018-08-15 09:16:46 +08:00
    @liyuhang 欢迎提 pr
    shijingshijing
        85
    shijingshijing  
       2018-08-15 09:17:49 +08:00 via iPhone
    @coolhubery 如果养成习惯了就真不错,大家共同进步,code review 还有一些计划类的 paperwork 其实从长远看都是必不可少的。如果为了赶工期某一个人开始写 smelly code,而且不加制止就 merge 进来,最终导致的是整个 project 散发臭味。就跟加班一样。

    ps:比较好奇你们的覆盖率达到 100%,是指需求覆盖率还是语句覆盖率?如果是严格按照 MCDC 做语句覆盖,每个条件都 cover 到,那 100%就厉害了。
    peterswan
        86
    peterswan  
    OP
       2018-08-15 09:18:18 +08:00
    @jiangjz 同感,有的代码看着让人头皮发麻,经过了 n 个人的修改,修改成本很高,只能重构。
    UIXX
        87
    UIXX  
       2018-08-15 09:20:48 +08:00
    有 CodeReview 要两点:
    1、领导强制力
    2、员工自觉性
    团队要有能人,员工要会进取、肯学习。其他都是扯淡。
    coolhubery
        88
    coolhubery  
       2018-08-15 09:28:36 +08:00
    @shijingshijing 语句 100%覆盖。除了极少部分遗留代码无法实现 100%的情况外,新增代码必须语句 100%,否则 Gerrit CheckBot 静态检查都过不了。
    coolhubery
        89
    coolhubery  
       2018-08-15 09:29:19 +08:00
    @fanqianger 西安...
    peterswan
        90
    peterswan  
    OP
       2018-08-15 09:29:45 +08:00
    @micean 就我的经历来看,靠自觉解决不了工程中出现 smelly code,只要有这种代码,就很有可能会让项目代码维护成本增高,而且如果维护的人写的代码继续糟糕下去,会使的项目走向慢性死亡,这些时间还不如拿出来放到开发的时候去 Review,其实是节省的整体项目的时间,管理人员更应该有这个意识。
    shijingshijing
        91
    shijingshijing  
       2018-08-15 09:43:41 +08:00 via iPhone
    @coolhubery 那这个成本很高了,我只在某些 safty critical 的软件上看到过类似甚至更严的方法,比如 pacemaker 产品不仅对覆盖率有要求,而且对实时性有非常高的要求,必须在指定时间内完成指定任务。

    你们测试是自己做还是外包给三哥?包括 test plan,test case,test report 都是怎么做的?
    Rico
        92
    Rico  
       2018-08-15 09:45:51 +08:00
    有。且代码评审作为升等的一个参数。
    thinkmore
        93
    thinkmore  
       2018-08-15 09:57:54 +08:00
    每次提交代码都要 code review.

    好处:学习别人写的好的代码,让别人指出自己写得不合理的地方或者造成歧义的地方。

    坏处:codereview 有时候时间太长,无法快速合并代码影响开发效率。
    coolhubery
        94
    coolhubery  
       2018-08-15 10:00:54 +08:00
    @shijingshijing SAP HANA 内存数据库执行引擎。
    因为是全新的执行引擎去替代老的引擎,所以大部分的端到端测试,性能测试,压力测试等都可以重用。自己的 change 提交的话是需要自己写 unit test 的,一些本引擎特有的特性需要自己额外写 E2E 测试。
    xoxo419
        95
    xoxo419  
       2018-08-15 10:07:28 +08:00
    人少 活多 事赶
    wujiyu115
        96
    wujiyu115  
       2018-08-15 10:09:36 +08:00
    阶段性 review,一个开发周期结束,上线之前 review 一下
    woofee
        97
    woofee  
       2018-08-15 10:55:02 +08:00
    @NCry 结对编程
    micean
        98
    micean  
       2018-08-15 12:38:25 +08:00
    @peterswan 本来也是半路接手的项目,项目本身的设计就有问题,从去年开始就跟上头不止一次提过要重构整个设计,以免项目越来越膨胀的时候彻底改不动了。然而一点卵用都没有,1 版本还在测试,2 版本的需求就过来了,3 版本的需求就开始规划了,设计思想基本没有,有点无力回天的感觉了
    peterswan
        99
    peterswan  
    OP
       2018-08-15 13:22:27 +08:00
    @micean 对于这种项目,上头的人如果只想着眼前开发的速度,不考虑可维护性,如果你本身也没这么大的权利去实施重构和审查,我觉得这个项目已经走向凉凉的道路了。如果能够实施,复盘重构是必须的了,重构加入代码审查,人力充足的话老版本的迭代和重构版本同步进行,员工心里写代码也会开心提高效率。
    mingyun
        100
    mingyun  
       2018-08-18 22:21:01 +08:00
    那么问题来了,有什么好的 code review 工具
    1  2  
    关于   ·   帮助文档   ·   博客   ·   API   ·   FAQ   ·   我们的愿景   ·   实用小工具   ·   965 人在线   最高记录 6543   ·     Select Language
    创意工作者们的社区
    World is powered by solitude
    VERSION: 3.9.8.5 · 34ms · UTC 20:46 · PVG 04:46 · LAX 13:46 · JFK 16:46
    Developed with CodeLauncher
    ♥ Do have faith in what you're doing.