我们所不知道的Code Review
Posted on September 29, 2009
Filed Under agile, essay | 9 Comments
也许是待得太久，就像被一桶草莓酱从头浇到脚，尝哪里都是甜味一样，当初次看到Code Review成为一个如此重型并且低效的活动的时候，我才知道，草莓酱的外面，是空气，裹着大地的气息，大部分无味，又或者是烟草味，或者汽车的尾气。
先看一看我们看到的一个代码审查过程：
- 开发人员领到任务。
- 一周之后，代码写完了。他觉得没底，需要找业务专家技术专家来评审一下。这个时候他代码还没有提交。于是他把本地所有没有提交的、修改过后的文件，放到一起，压缩成rar包。找到他认为的技术高手业务高手（们），定会议室，发邮件通知。2小时过去了。
- 技术专家业务专家收到了邮件，由于缺乏上下文理解，以及长达数千行的源代码，这类邮件一般不看——因为看了也是白看。
- 终于到了Code Review的那一天。七七八八的来了几个人。一般来不全。因为高手之所以是高手，表现之一就是超级忙。
- 于是代码的作者开始，一行一行的将代码讲下去。前几十分钟高手们也没办法理解——毕竟是一个星期的工作沉淀，哪有那么快理解的。大约30分钟之后，专家开始提出建议意见。这些意见一般涵盖了语法、编码规范、可能的业务错误、模块间关系等。专家们毕竟是专家。2个小时之后，专家们离去。
- 开发人员虚心的把这些意见、建议写到小本子上。
- 开发人员可能根据专家的建议进行相应的代码修改并且提交，也可能不；可能改对了，也可能不。评审过后，后续的实施成为黑洞……
没了。
先思考一下，这个过程中的问题。
======== 思考的分割线 ========
首先必须承认Code Review的价值。经验丰富的专家们在做代码审查的时候，能够根据以往经验，规避重大缺陷的发生，对开发人员给予有价值的指导。然而，这个过程，太冗长，太低效。
- Code Review必须基于事实。这里的事实，就是，源代码库。SVN Repository, 或者HG/Git Repository. 在多人协作环境中，对于一份不在源代码库代码是基本不可信的 —— 你无法预知，他是否将会成为最终可工作软件的一部分。
- 积攒下来众多的代码修改，使得产生重型、低效的沟通方式成为必然。这类众型的沟通方式往往成本惊人 – 需要占用最好的人很长时间。
- 过分夸大专家的作用。根据以往经验，许多最终发现问题，回溯上来，其实是一些简单的逻辑问题。这些问题如果分散在平时结对或者更频繁的过程中，则更容易发现。很多情况下，是开发人员对常见的bad smell了解和修炼不足，而这些bad smell常常是导致问题的地方。例如在一个已经有3重循环的方法中加入了新的判断而没有测试；修改了函数的返回值而没有任何说明；if 判断中包含了多达4-5个变量的比较判断而没有抽象为一个更具业务含义的方法，等等。
- Review手段的原始落后。Review必须基于变化。会看报表的人都知道，看报表只需要看两个东西：趋势和拐点。Code Review也一样，只需要看变化。SVN/Hg/Git这类现代化的工具给我们提供了丰富的，基于changeset的compare工具。查看一天，整个团队的check in情况，顶多只需要10分钟-15分钟。
在敏捷过程中，Code Review几乎是一个被忽视的环节——不是不做，而是时时在做。结对时，我们会对结对伙伴的编码习惯、新写的类、变量表示质疑；提交之后，有代码静态检查工具、单元测试工具、覆盖率工具帮助我们检查有没有犯简单愚蠢的错误、有没有破坏既有功能；持续集成服务器则中立、不知疲倦的在每次我们提交之后运行所有的过程。
Code Review不是一个审查环节。不是一个考核环节。它是交流和反馈环节。
Comments
9 Responses to “我们所不知道的Code Review”
Leave a Reply
看来你在大公司混得时间短，经受的催残少。:)
正经的。这里的Code Review，和我们理解的Code Review稍有差别。所谓专家，看的是业务上有没有遗漏，对于代码的Bad Smell很少嗅到，或是容忍度太高，根本没有人知道好代码应该是个什么样子，这才让代码的味道越来越大。
严重同意！
Pair-Programming不就是减少Code-Review – Re-Review的工时的方法吗？
Inline checker不也是辅助检查并减少code问题的有效工具吗？
感慨！一方面，这世界上有些人在努力地变革，寻求更好的方法来解决现实中的问题。另一方面，有些人在因循守旧，并且将勇于变革视为毒草猛兽。扼杀创新。
准备跳了。
Will your company accept human like me?
呵呵，当然欢迎了。但是，软件技术是一回事，作为组织由此产生的盈利是另外一回事。很多组织笼罩在盈利能力的光环下，所谓一俊遮百丑，由此之下的过程、工具、技术往往就不够看；而另一方面，一些有想法、过程优秀的公司，却规模大不了……这世界永远都这么公平。
dreamhead>>
Bad smell没有人管，不是因为大牛不看，而是所谓的大牛们根本看不出来。
大牛们需要按照过程定义一步一步的执行。至于每一个过程什么是正确的，什么是优秀的，根本就没有定义。而只是定义了过程需要什么工作。
举例来说，他们会定义一个比较莫名其妙的目标：千行代码Bug数，CodeReview必须发现多少，Unit Test发现多少，Integration Test发现多少。而且必须是一定的范围，理由是：发现太少是因为Review不充分，发现太多是因为编码不合理。最后，导致交货以后，被客户发现的不合格率很高。另外，对于测试覆盖率的要求居然不是路径覆盖率和条件覆盖率等，要求的是，每千行多少个测试点。
所谓过程的公司更多的是关心过程的执行有无，而不是过程执行的合理性。
我想我应该写一篇文章：SQA does nothing in software developing procedure.
盈利的好办法就是保留Bad Smell。因为这样用户用一段时间不爽了，就要求变更需求。这个时候，就可以开始估计新的工作量，而工作量当中包含了很多前一次开发就可以防止的事情….返工还要跟客户收钱。
>>>>>>>>>>>>>>>>>>>>>>>>>>>
agile很适应产品开发，因为节省成本，易于变更是为自己服务的。
而定制项目更适应与CMM开发，因为各种报价依据充分，有说服力。看起来很规范。
Agile或者Refactoring,Design Pattern等技术方法更适于对已有项目的改造，告诉客户我们的比以前的好。
<<<<<<<<<<<<<<<<<<<<<<<<<<<
举例来说，他们会定义一个比较莫名其妙的目标：千行代码Bug数，CodeReview必须发现多少，Unit Test发现多少，Integration Test发现多少。而且必须是一定的范围，理由是：发现太少是因为Review不充分，发现太多是因为编码不合理。
看到这段我真是又想说好,又想说我cao,又想哭.
合理的应该是诸如Unit Test Coverage 100%之类的目标。
这个指标达到了，一定程度上说明了质量的状况。
但是发现多少Bug根本就是扯淡，高手做的程序Bug很少，新手满篇都是Bug，凭什么定义一个统一的指标？
有的公司也挺有意思，定义了一个多少年工作经验的人的指标是多少。这就跟说多大岁数才能升到什么位置一样可笑。