Rails 请带团队做过 Code Review 的大神介绍经验

Blues · 2013年10月24日 · 最后由 Blues 回复于 2013年10月25日 · 5320 次阅读

RT,我觉得我们团队需要做一点 code review 的事情,希望团队成员的代码写的更好一些,算是给他们一点压力。

本身我的主职不是做 rails 开发,但是我对 rails 还是很感兴趣的,也算入门了。只是对做 rails 的 code review 还感觉信心不足,但是对于 OS X/iOS C/C++ 项目的 code review 没问题。现在的问题是团队有一些项目是 rails 的项目,其他的 rails 的成员我觉得也不足以完成 rails 的 code review 这个任务,所以想请大神支支招。

不止 rails,如果大神们还有一些其他平台的 code review 经验,也非常欢迎讨论。

谢谢!

这问题跟 code review 有个毛线关系,分明是写测试的问题,修 bug 的时候怎么不写相关测试代码……

#1 楼 @aptx4869 谢谢,首先我承认测试可以解决这个问题,测试也要写,我只是拿这个问题举例子(可能举得不太好....... :-(.......),解决这个问题的办法有很多,我想问的不是怎么解决它,而是怎么做 code review,请大神介绍下,谢谢!

@aptx4869 @raven @mouse_lin 重新编辑了下帖子,突出下重点,谢谢!

#3 楼 @Blues 如果你们没有一个强悍的LEADER来做这个,那么就要求大家互相看,并且需要把 code review 的时间计算入工作时间,这样大家做起来才不会有压力。code review和测试是挺密切的,你所 review 的代码肯定要包含测试,因为测试本身也是代码。。

#6 楼 @raven 谢谢,对于 rails 我们确实没有一个非常强的 rails leader,把 code review 计入工作时间没有问题。我说的给他们“压力“,是指他们的代码现在是有人看的,写的不好大家是有目共睹的,是指的这个压力,测试的代码我已经要求 rails 的 developer 做了,我检查了下,这个已经确保是有的,只不过我目前保证不了测试是全面的。

我的意思是我会单独拿出一天或者半天的时间去做这个事情,让他们就在那个时候互相看还是提前给时间让他们看,然后 review 的当时写测试代码?

或者是我们拿出一段觉得可以优化的代码,然所有人一起讨论?

#7 楼 @Blues 我的经验是 例如小组里有ABC三个人,A的一个 PR(pull request) 做好了。那么就叫 b 和C去看,然后 b 和C至少有一个人去 review 然后直接在 PR 里评论(我们使用 github),其它人都会看到这个评论 如果有意见也可以一起来。这个不是固定到哪一天去做,而是有需要的时候去做。

一个 PR 想要 merge 最基本两点就是 reviewed and CI passed.

@raven 非常感谢!我以为需要拿出一整天的时间单独做这个事情,现在看来,这是在准备 deploy 的的一项基本的工作。有点明白了,谢谢!

#9 楼 @Blues 严格控制 production 分支的质量 :p

#10 楼 @raven 嗯。Thanks!

#8 楼 @raven 我记得 teahour 的采访里讲,他们团队是互相看的,一个人的代码要另外两个人看。但是那些都是形式。

讲讲我的思路: 首先,业务逻辑的 code,需求 ticket 要拆分成技术点,每个技术点如何实现写到 issue 里,这样,写的人和分配到 issue 的人,都清楚要干什么。 然后就是按照技术点去 review 是否符合要求。这个工作很耗时但是从团队协作和质量控制上,这个时间是要投入的。 至于测试,这个可以有专人维护(如果人力够的话,我是建议专人维护的,这样可以保证测试用例的连贯和测试覆盖程度高),如果个人维护,review 先从测试开始,再看代码,方便理解如何实现的。

review 测试我更看重的是逻辑覆盖程度。当然,具体情况具体分析。

如果是 rails view 的改动,因为单元测试意义不大,view 的测试在回归测试时重点检查。

我非常看重 code review 这个环节,可以让团队更行动一致。也可以阻挡一下需求对 code 的直接影响。毕竟太多人的坏习惯是跟着需求做,最后一团乱麻。当然啦,赶工的除外。。。。赶工的东西一定要找时间重构的。

code review 包括两种,一个是高级评审,一个同行评审 高级评审是高手对菜鸟的,一般由技术经理来进行,重点是看这个工程师写的代码是否合格。 同行评审是技术人员之间的,重点是代码的交流,代码互通,风格互通。 当然评审本身的目标,找 bug、提高代码质量还是不少的

#13 楼 @ZombieCoder 我觉得,对于评审的人也是自身学习的过程,协同开发,每个人都要从别人那了解各自的思路,思路太独特,就要用业务流程和设计规范来约束下。 我认为 manager 的很重要的事情就是设计开发思路。约束开发行为。

#12 楼 @liwei78 谢谢!有一点我不太明白的是:技术点需要写到 issue 里面去?另外对于“按照技术点去 review 是否符合要求”,受教了,我一般 review 都是通过应用表现出现的功能是否符合要求,以前没太管技术点的实现情况。 @ZombieCoder 看来对于 rails 的 review 我们团队目前适合同行评审。谢谢!

#15 楼 @Blues 举个例子,书上的例子。

一天,客户说,我们订书的系统,对于 VIP 用户,订购满 30 元免运费。快加上这个功能,我们明天用。

那么技术团队怎么应战????

首先这个需求,PM 必须拆解,比如 1,我们改动那个 model,2,我们改动那个 controller 的那个方法,增加哪个判断,3,这次改动对应的流程有调整,按照 xxx 流程图补充测试,4,我要跟客户确认:如果它买了一个冰箱,再买了 1 本书,还免运费么?

至于代码细节大家也都编程,或许都能想到了,不过像 4 里面的问题,如果在 coding 的时候才发现,再去找客户,恐怕就来不及了。还有,对于一个不了解需求的“新”人或者刚睡醒的“老”人,拿到拆分好的 issue 是很幸福的,完成快,测试人员也不会打回 issue。

要知道在华为,issue 被退回两次是要被调岗或者停职的。。。。。。

#17 楼 @yedingding 这是敏捷开发很好的总结。当然不敏捷,又如何,毕竟代码能正确工作,谁还能说它什么呢。

#18 楼 @liwei78 具型的可以听 Terry 在 Teahour 讲的,我们团队就是那样子做 review 的

#16 楼 @liwei78 例子很直观,谢谢!issue 被退回 2 次就调岗或者停止,这叫我情何以堪呐。。。 @yedingding 谢谢!快读看了下,我想问的是介不介意我把你的这篇文章拿出来讨论一下,然后按照我们的情况适当修改一下?

#18 楼 @liwei78 参考<程序员的自我修养>

#19 楼 @yedingding 听过的,很受用。是这期:http://teahour.fm/2013/10/21/talking-remote-work-with-allen-wei.html。想远程工作的话必听。

#22 楼 @liwei78 喔,是远程工作那一期,#35 吧。Kevin 和 Terry 采访 Allen

#18 楼 @liwei78 @yedingding 谢谢! @liwei78 很多时候程序员们都会有些“癖好”,这一般都是高手的习惯,他们会认为代码能正确工作并不是 100% Ok,而是让他觉得所有代码他 (他们那一类人) 看来都是 Ok 的才是真的 Ok,而他们把这些“癖好”,汇编成一些基本的规范让人们去学习,不可否认的是往往 (不一定是 100%) 他们说的是对的,对于项目来说是有利的。 @yedingding 只听过,没看过这本书。我内心认为我上面的理解就是这本书所要讲述的问题。不过有有时间我还是会读一下,来确认一下我的看法,哈哈!

#24 楼 @Blues 嗯,精神洁癖,我们团队很严重。哈,我没看过,我只是想起了星爷的<演员的自我修养>

#24 楼 @Blues 这种有决定意义的人很重要的,团队里非常需要这种人,有点小问题也就无所谓了。呵呵。。 #25 楼 @yedingding 我也是精神洁癖,我受不了没有流程图,没有测试的东西摆在我面前。我倾向于小而精的产品。

#26 楼 @liwei78 我们从不画流程图,我们不追求 100% 覆盖率

#27 楼 @yedingding 那你们怎么整理需求和把需求变成 code 的呢???小伙伴们一起来偷窥下。。哈哈哈

#26 楼 @liwei78 对啊,这个人确实关键,如果找不对这样的人,对于团队来说是个灾难啊,所以目前最好的办法就是自己成长为那样的人。流程图感觉不是那么重要,一般都是给出一个 mockup,然后大家就知道流程了,感觉 mockup 就替代了流程图了。 @yedingding 个人认为 100% 的覆盖率过于困难了,但是一个项目长久下去,其实还是在朝着 100% 的覆盖率前进,只是它是一个缓慢的过程而已。

#28 楼 @liwei78Pragmatic.ly 啊,为什么需要流程图?变成 code 的话,来 RubyConf 听我们团队的 Roy 讲《Discovering Better Object Oriented Design with Tests》

#29 楼 @Blues 是的,我的想法是有点进击。。。不过我之前的经验就是,流程要落到纸上,这样大家都有迹可循。定下来的东西就不要轻易改。这是 11 年一个项目的教训。

覆盖率 100% 是不可能的,越到后期越不可能,不过核心逻辑还是要 100% 的。if...else..多了真是灾难。

重构的基础是测试,不接近 100% 测试的重构有风险。。

#29 楼 @Blues 嗯,Mockup 或者 Feature

#30 楼 @yedingding 好的,一定会有收获。。

#33 楼 @liwei78 哈,明晚见,:)

#34 楼 @yedingding 明晚 9 点到北京,进二环也得 10 点以后了,哈哈哈哈。你们先玩吧。后天我早上去会场。

#31 楼 @liwei78 任何时候有了教训之后得到的经验一般是很难改变的,我理解这种情况,我自己也会这样,但是后来回过头,仔细想想,“好像是我把承担责任的对象搞错了,根本不是那个的错,如果怎么怎么做就可以避免这个问题了”。

#36 楼 @Blues 我更倾向用“公约”的形式做事。比如,说过的事情是有文档可查的,确认的逻辑有文档描述,如果及其复杂,那么,需要很大的耐心来讲解其中的关节之处。这样能做到透明。。。。当然啦,这样做事很累心。。

#37 楼 @liwei78 根据团队规模和公司的规定不同,其实只是一种习惯。成规模的公司大部分都是你说的这样的,做事情一定要有依据,小团队可能不会这样,一个是时间因素,你知道,那是很累心的,另一个是必要性,极端例子,如果团队只有一个人开发,除非后来有大批招人的计划,否则那根本就没必要。

#38 楼 @Blues 是的,协作的时候每个人观点都不同,如果不控制,那么必然引起矛盾。控制的方法我倒不建议自我约束,我更倾向强制约束,也就是按照敏捷要求走,那么敏捷也有很多方式,那就用一种大家都认同的方式。 在重构项目时,约束更重要。

“原因就是修改 bug 的代码被后来 push 的代码覆盖了”

这是怎么做到的

#40 楼 @sdpfoue 话说我不是已经编辑了主题了么?为什么还可以看得到?

就是一个 master,然后 branch A,branch B,branch A fix bug,然后 branch B merge,conflict 中,直接采用了 branch B 里面的代码,把 branch A 里面的修改 bug 的代码直接去掉了。所以就.......

#41 楼 @Blues 这不是 100% B 的问题吗。有冲突不看代码就抛一支。话说这种人我也碰到过一次还死不认账,在 git 面前都是徒劳

#42 楼 @sdpfoue 对啊,是 branch B 的问题,但是由于沟通不够,branch B 不知道那些跟他不同的代码是干什么的,然后也没有人进行 review,所以才出的问题,所以我觉得@raven的办法很好。

需要 登录 后方可回复, 如果你还没有账号请 注册新账号