职业IT人-IT人生活圈

 找回密码
 成为会员
搜索
查看: 603|回复: 8

讨论一下codereview的变种

[复制链接]
broken 发表于 2011-8-21 11:20 | 显示全部楼层 |阅读模式
    公司终于下定决心要推广code review了。我理解的原生code review 应该所有人互相check所有代码,主要目的是提高整体的代码质量。公司考虑时间成本和工程师们水平参差不齐,决定如下搞:
每周经理会从已经完成的代码当中选择一个人或两个人的代码,然后大家坐在一起看,找问题提建议,共同学习,时间限定为两个小时。之后大家再根据交流结果回去修改自己的代码。
    公司的目的是省时间,这种方式对提高大家的整体水平很有帮助,也弥补了单纯互相看由于个人水平限制找不出问题的局限。但我担心单纯讨论的话可能会有下列的问题存在 :
    1. 缺乏代码覆盖率,一周讨论内容只能覆盖一小部分部分代码,大部分代码的潜在错误都无法发现。难于保证整体代码的质量。
    2. 一般人精力最多集中1个半小时,一次讨论两小时过于长了。开到后面,很容易变成大家闲聊和天马行空想法的讨论,对代码质量把关的意义不大。
    3. 互相check一定程度上保证了大家对彼此功能的业务、实现细节有了解,而统一讨论一次只能让大家了解一小部分。
    4. 讨论会后要根据讨论结果各次修改自己犯得同样问题。积攒了一周的代码要复查和重构也不太容易
    5. 人都容易看出别人的缺点,不容易看到自己的,这也是要互相reviewk的原因。讨论会上虽然能统一问题和修改方式,但做了类似事情的个人可能察觉不到自己也犯了类似的错误。
    6. 要每周确定一次够全体人员讨论两小时的代码,对经理的时间和精力要求过高。要是经理忙,是推后、找别人代找,还是随便找段让大家讨论?
    我觉得考虑如果兼顾统一讨论和互相review会不会效果更好些:
    1. 讨论会改为一周一次半小时。经理明确一段有问题的代码,大家快速讨论达成共识。代码只需要够半小时讨论,这样对经理的要求低一些,会议时间和内容也好把握。
    2. 每天五点钟互相review15分钟。
       (1)review当天五点前提交的代码,之后提交的代码不参与当天review。需要注意的是提交的都是通过单元测试、merge回trunk的代码。
       (2)五点开始,根据svn版本号区分,每人review别人提交的一个版本改动,用时不超过十五分钟,通过“喊话机制”/电话保证review版本互相不重,如当天提交的代码不够review,可以review前几天提交未review的代码。
       (3)每次review先要花几分钟快速了解被review代码的作用 ,每个代码提交者提交代码时需要理用简单几句话描述自己的提交都做了什么,有什么注意点。
       (4)到15分钟必须结束,必须要给出一个review意见,哪怕是“没发现问题”。
       (5)review的svn版本,review人,review评价要汇总记录下来,个人需要查看自己提交的版本号被review结果,自己把握要修改哪些;经理整体把握review内容,有权根据review意见强制个人修改。
       (6)reviewer可以根据实际情况认为自己意见是必要要修改的,通知被review人,并在review意见里明确重要。比如review结果用 execl记录,有一列写的重要与否,用标红表示;再有一列记录结果,比如“经理认为不重要”,确认已修改,确认还未修改等等。
       (7)如果经理忙没空做每周的代码查找,则拿出review记录,讨论标为重要的内容。
       (8)review完,该下班的下班,该工作的工作,想吃饭的吃饭,该干嘛干嘛。
        这样对每个人来说,每周花在review大概也是两小时,还很好利用了每天下班前大家开发效率低的时间。

叫我小乖 发表于 2011-8-21 11:20 | 显示全部楼层
应该先用一些工具检查一下,如代码规范检测,代码重复检查这些

Jethro 发表于 2011-8-21 11:20 | 显示全部楼层
dohkoos 写道
应该先用一些工具检查一下,如代码规范检测,代码重复检查这些

代码规范 我们用checkstyle,代码重复检查用什么?

顺便说,我们checkstyle有个很让我不舒服的设置,不能有行尾空格,也就是说,行尾必须是用字母,符号结束而不能有空格。
我个人觉得,style是为了人看的,有没有空格真的不影响人看。
在eclipse下(我们原则上不能用eclipse要用vim),方法注释的倒数第二排带空格,本意方便写注释的,要么修改底层注释模板重新编译eclipse,要么的手工一个个改,要是用了自动梳理格式,又回去了....用vim的话可以设置保存自动去掉行尾空格,但会失去之前的焦点,对于喜欢频繁保存的人影响很大...

能文能武 发表于 2011-8-21 11:20 | 显示全部楼层


楼主这样不行,最好搞结对开发,让对方相互review对方的,出错了很容易找到谁没有认真review.另外,应该找些高手来review那些senior的代码。
另外,每天review代码有时是做不到的,客户啊,进度啊会打乱这个,
最好你说的:
(8)review完,该下班的下班,该工作的工作,想吃饭的吃饭,该干嘛干嘛。
如果是我在你的团队里,我会很不爽你这样。我是这样管理的:mentor制度,高级开发的带初级开发的(结队),如果初级开发的工作没完成,那么高级开发的要陪他一起搞定:给其讲原理,方法(这样最大的好处是,对新员工的提升很大,也促进了员工间的和谐,你想,如果我是个新人,当我的工作没完,大家都走了,我肯定很不爽,如果有个老员工陪我一起搞,我会很努力很上进的。这样带team,最好了)

叫我小乖 发表于 2011-8-21 11:20 | 显示全部楼层
crowgns 写道
dohkoos 写道
应该先用一些工具检查一下,如代码规范检测,代码重复检查这些

代码规范 我们用checkstyle,代码重复检查用什么?

顺便说,我们checkstyle有个很让我不舒服的设置,不能有行尾空格,也就是说,行尾必须是用字母,符号结束而不能有空格。
我个人觉得,style是为了人看的,有没有空格真的不影响人看。
在eclipse下(我们原则上不能用eclipse要用vim),方法注释的倒数第二排带空格,本意方便写注释的,要么修改底层注释模板重新编译eclipse,要么的手工一个个改,要是用了自动梳理格式,又回去了....用vim的话可以设置保存自动去掉行尾空格,但会失去之前的焦点,对于喜欢频繁保存的人影响很大...



我个人经验觉得FindBugs比checkstyle更实用一些, 能发现很多值得修改的代码缺陷。

北大青鸟 发表于 2011-8-21 11:21 | 显示全部楼层
四书五经 写道


楼主这样不行,最好搞结对开发,让对方相互review对方的,出错了很容易找到谁没有认真review.另外,应该找些高手来review那些senior的代码。
另外,每天review代码有时是做不到的,客户啊,进度啊会打乱这个,
最好你说的:
(8)review完,该下班的下班,该工作的工作,想吃饭的吃饭,该干嘛干嘛。
如果是我在你的团队里,我会很不爽你这样。我是这样管理的:mentor制度,高级开发的带初级开发的(结队),如果初级开发的工作没完成,那么高级开发的要陪他一起搞定:给其讲原理,方法(这样最大的好处是,对新员工的提升很大,也促进了员工间的和谐,你想,如果我是个新人,当我的工作没完,大家都走了,我肯定很不爽,如果有个老员工陪我一起搞,我会很努力很上进的。这样带team,最好了)



我这里结对开发,互相review的时候。发现如果两人资历差不多的时候,互相review都会不太认真,好像不愿得罪对方的样子。老员工review新员工效果最好,老员工容易认真,新员工也容易听进去。

另外,我觉得利用工具比人工review效果更好。
我是要求大家把eclipse里的编译警告都打开,做出来的代码不要有任何警告(虽然有时候不现实),这样就已经能减少不少代码缺陷了,再跑跑findbugs, 也是要求把findbugs发现的缺陷全部改掉。 然后再用metrics等代码分析工具分析一下,最后用profiler看看性能瓶颈,做完上述工作后代码质量基本是可以能接受的范围了。

shmilyyu 发表于 2011-8-21 11:21 | 显示全部楼层
jak47 写道
四书五经 写道


楼主这样不行,最好搞结对开发,让对方相互review对方的,出错了很容易找到谁没有认真review.另外,应该找些高手来review那些senior的代码。
另外,每天review代码有时是做不到的,客户啊,进度啊会打乱这个,
最好你说的:
(8)review完,该下班的下班,该工作的工作,想吃饭的吃饭,该干嘛干嘛。
如果是我在你的团队里,我会很不爽你这样。我是这样管理的:mentor制度,高级开发的带初级开发的(结队),如果初级开发的工作没完成,那么高级开发的要陪他一起搞定:给其讲原理,方法(这样最大的好处是,对新员工的提升很大,也促进了员工间的和谐,你想,如果我是个新人,当我的工作没完,大家都走了,我肯定很不爽,如果有个老员工陪我一起搞,我会很努力很上进的。这样带team,最好了)



我这里结对开发,互相review的时候。发现如果两人资历差不多的时候,互相review都会不太认真,好像不愿得罪对方的样子。老员工review新员工效果最好,老员工容易认真,新员工也容易听进去。

另外,我觉得利用工具比人工review效果更好。
我是要求大家把eclipse里的编译警告都打开,做出来的代码不要有任何警告(虽然有时候不现实),这样就已经能减少不少代码缺陷了,再跑跑findbugs, 也是要求把findbugs发现的缺陷全部改掉。 然后再用metrics等代码分析工具分析一下,最后用profiler看看性能瓶颈,做完上述工作后代码质量基本是可以能接受的范围了。



code review的终极目的是什么? 说到底,是为了项目的质量管理。
楼主所说的,互相check所有代码,这在小项目是可能的,但是项目一大,当代码量较大时(几十至几百M,甚至上G),是行不通的。楼上所说的用工具来操作,恐怕当代码量太大时,也不大行得通。
所以,当代码里较小时,用楼主所说的,相互check所有代码(我不赞成check所有代码),或者用结对开发来check
当代码量中型时,就用楼上所说的,工具来操作+结对check
当代码里大型或巨型,恐怕得将代码分模块来review(工具来操作+结对check),再在此基础上,用PMBOK里提到的,加上统计与抽样来对质量进行控制了。

话说我当年 发表于 2011-8-21 11:21 | 显示全部楼层
首先,开发者要自己养成review前使用findbug等工具扫描代码的习惯,再提交review。
时间上,code review建议一周做一次,每天一次时间上不一定能保证,比如开会、问题讨论等都可能把code review时间占去。
形式上,code review建议由架构师来review代码,采用老手+新手方式结对review,老手之前的代码再相互review,技术经理定期抽查代码进行review,并监督review执行情况。
工具上,建议使用Jupiter等code review工具来进行,这样便于跟进code review的执行情况,也方便开发人员快速进行code review,减少沟通成本。
最后,定期(比如每两周)对review过程中出现的常见问题组织code review会议进行总结和学习,避免再犯同样错误。


郁闷小男人 发表于 2011-8-21 11:21 | 显示全部楼层
首先做静态代码检查,Findbugs, PMD, Checkstyle三种各有各的偏向性,可以混合来用,配合CI工具例如Hudson。这样可以减少人工成本。
然后开始做Code Review。Code Review一般来说有三种,可以见《代码大全》,这三种我们都实践过,最终觉得先让每个人在线下互相Review,然后组织会议一起过一遍这样效果比较好。一方面不会让会议时间太长而导致效果下降,又可以兼顾分享问题。我们之前也用的是Junpiter。
楼上所说的结对也是一个方式,但结对实施成本太高,而且取决于结对的两个人是否能够很好的配合。
您需要登录后才可以回帖 登录 | 成为会员

本版积分规则

QQ|手机版|小黑屋|网站帮助|职业IT人-IT人生活圈 ( 粤ICP备12053935号-1 )|网站地图
本站文章版权归原发布者及原出处所有。内容为作者个人观点,并不代表本站赞同其观点和对其真实性负责,本站只提供参考并不构成任何投资及应用建议。本站是信息平台,网站上部分文章为转载,并不用于任何商业目的,我们已经尽可能的对作者和来源进行了通告,但是能力有限或疏忽造成漏登,请及时联系我们,我们将根据著作权人的要求立即更正或者删除有关内容。

GMT+8, 2024-3-29 13:08 , Processed in 0.134080 second(s), 20 queries , Gzip On.

Powered by Discuz! X3.4

Copyright © 2001-2021, Tencent Cloud.

快速回复 返回顶部 返回列表