CodeReview的几点思路

这周在公司一直给同事做CodeReview,感觉还是比较痛苦的,因为一些机制并不是很人性化,至少说,流程上有一些不成熟的环节。和大家一起分享,希望有经验的朋友给我一下批评建议。

方案1:Review Meeting,即Online Review。
基本不可行。很大程度上依赖于主持者的素质,而且如果主持者不积极,就会流于形式。此外时间成本过高,为reviewer+owner的时间总和。倒是可以将其改造为Team内部新技术新观点的交流Meeting,完全头脑风暴式的那种。

方案2:Offline Review
我特别想说的就是这种。包括重构、代码逻辑、代码规范。

对于变更的代码,owner应该提供一份ChangeList,基于需求或者功能点,列举出增删改动了那些类和方法以及关键变量,这就迫使owner首先review一下自己的代码,从而首先自身发现一些问题。对于代码重构,更需要这份清单,甚至是一个变更前后的类之间的关系图(当然可以从之前的LLD中复制过来)。额外的一个好处就是,重构会把很多方法原封不动的从一个类迁移到另一个辅助类,那么,reviewer就不用去care这些代码,而只是关心整体的结构,从而对症下药,对架构而不是代码,提出新的见解。

Code-review并不应该局限于纸面上,仅仅依赖于一个DB是不够的。一定要进行face to face的沟通或者by phone。由reviewer来讲解自己的心得,可以不受owner的主观意向影响,从其他角度来思考这个问题,还有就是迅速掌握代码,可以在owner离开的情况下迅速接手这段code;由owner来讲解,可以在介绍过程中,及时发现一些无法自圆其说的地方,然后加以修正。因此code-review从准备到完成的时间,应该大致为coding的50%左右。

最后,非常不赞同连错别字都写进code-review报告的行为。一方面要肯定reviewer的勤恳,但是,另一方面,这些与code无关的suggestion,会转移我们的视线。我们究竟要关注什么?是代码质量,是架构设计,而不是单词拼写错误。过多此类的suggestion,会把真正的问题掩藏起来,即使有critical等级的区别,也无济于事。一种极端的解决方式是禁止提这样的suggestion,比较缓和的方式是私下交流这些小错误。
额外需要指出的是,对代码规范的审核,尽量不要依赖人力,而是通过先进的工具来处理。人,总是要做一些机器做不了的事情,比如说重构与算法的review。这样,我们就可以有更多的时间focus在这些高层次的地方了。


补充:版本变更的代码比较(我们公司使用的是ClearCase)
如果类中原先有两个方法A和B,A在B的前面。版本变更后将A方法挪到了B方法的后面,那么ClearCase会只认为B方法是不变的,而认为新版本在B方法前删除了A方法,而在B方法后又添加了这个A方法——这就由ClearCase的逐行比较算法导致的,它毕竟只是一个文件控制工具,而不是for code file的;但是对于人而言,其实是没有改变的。这就对Code-Review添加了困扰,如果代码文件2万行,将一个方法从头位置挪到了尾部,这无疑就给reviewer造成了麻烦。

解决方案:如果ClearCase比较不同版本的代码文件的算法,能够细化为先list出一个类所有的成员(使用反射),按字母顺序排列,那么比对两个版本的类文件时,就可以按照成员的顺讯,先比较成员是否有所增删改变,再深入到方法属性中,用ClearCase原先的算法,逐行比较代码变更。
此外,还要注意嵌套类,因此要使用迭代来实现以上新算法。还有就是partial分散类的问题,这应该在代码规范中,禁止使用这种类的实现(自动生成的winform窗体除外)。

posted @ 2008-06-12 17:19 包建强 阅读(2068) 评论(11)  编辑 收藏 所属分类: Others

  回复  引用  查看    
#1楼 2008-06-12 17:29 | 小寒      
code-review是不是可以分层次来进行
浅层次的review只是检查代码是否符合规范,层次是否正确
深层次的review来检查代码的质量,重构等

这样分开来做,可以让经验不是很丰富的人来做浅层次的
让像LZ这样的有丰富经验的人来做深层次的

这样大家的关注点不同了,工作的目的性就更明确了
  回复  引用    
#2楼 2008-06-12 17:58 | 潜水员甲 [未注册用户]
除了可以借助一些工具外,最好还可以将code review的工作固化在流程中。
  回复  引用    
#3楼 2008-06-12 20:04 | top10-it [未注册用户]
代码重构?
  回复  引用    
#4楼 2008-06-12 22:13 | ant007 [未注册用户]
看来贵公司的codereview水平仍处于cmm二级水平

1:review会议一般有两种操作方法,第一种是review介绍会议,应用于大型项目, reviewer唯外部专家或本组内对项目了解较少的人, 在正式review前需要对他们进行扫盲;另外一种就是review确认会议了, 大家提完意见,进行第一轮确认后开会讨论有争议的意见

2: 一般review代码都是offline的,在线评审的一般是方案, 在线评审代码不好操作.
流程中必须要求coder自己先review,review的缺陷要记录, 如没有该记录或缺陷率不够不允许进行外部评审

拼写不算错误说明你们的软件对质量要求不高

代码比较工具可以用beyondcompare, 一般不用clearcase review

欢迎讨论
  回复  引用  查看    
#5楼 2008-06-13 10:35 | yzlhccdec      
CMM不是每个地方都适用....
  回复  引用  查看    
#6楼 2008-06-13 10:42 | Tony Zhou      
beyondcompare不能结合版本控制吧。
  回复  引用    
#7楼 2008-06-13 10:54 | etng [未注册用户]
reviewboard
Rietveld
等在线review工具也好啊,可以和现有的SCM集成,只是clearcase貌似是商业化的工具,可能没有集成。
  回复  引用    
#8楼 2008-06-13 10:58 | Tiankong [未注册用户]
CMM级别与CodeReview没什么必然的联系

小寒的建议还要添加上Review人的责任心
  回复  引用    
#9楼 2008-06-13 17:40 | 过客001 [未注册用户]
我们究竟要关注什么?是代码质量,是架构设计,而不是单词拼写错误。--到代码审查才关注架构设计似乎太晚了,拼写错误多也是影响代码质量和编程效率的因素。也不容忽视。对已完成的代码进行结构上的讨论沟通成本太高,没有人会承认自己的算法有问题
  回复  引用  查看    
#10楼 2008-06-13 17:50 | 张树坤      
@ Tony Zhou
beyondcompare可以结合版本控制工具,再cvs或svn中设置第三方的比较工具即可。
@lz
个人认为review主要是发现有没有系统层次上的调用错误,大体逻辑是否正确;代码实现和设计是否合理。
想要对架构提出改进建议,首先要对系统足够了解,成熟的系统没有2年一般是提不出什么好的建议的。
  回复  引用    
#11楼 2008-08-12 22:24 | MEsss [未注册用户]
CMM已经被证明一开始就是一个错误,CMM5都是忽悠钱用的。

标题  
姓名  
主页
Email (只有博主才能看到) 
验证码 *  看不清,换一张 [登录][注册]
内容(请不要发表任何与政治相关的内容)  
  登录  使用高级评论  新用户注册  返回页首  恢复上次提交      
该文被作者在 2008-08-05 01:15 编辑过


相关链接: