Things Everyone Should Do: Code Review

2012-02-15 wcdj

这是一篇Google员工关于cr的看法，文中有几个观点比较有道理，翻译分享下：
(1) Google的代码质量好的原因很简单，就是cr，每个人都需要做，而且不只是针对product code，用少量的cr成本换取更大的收益。
(2) cr的好处显而易见，除了别人可以帮你发现程序中的bug外，最大的好处是purely social，而不是每个人都埋头苦干，我们可以从别人的视角学习到新的东西,这样更有利于知识的传播。
(3) 另一个好处是，cr可以避免每个人都使用私有的代码libs，每个私有libs就开发者自己熟悉，倘若有一天换了人，no one konws anything about it!
(4) 当然任何事情都不是容易的，如何使用好cr要在实践中不断摸索。
(5) 如果工作中由于种种原因无法进行cr，自己作为owner就要严格要求自己，多向有经验的programmer取经。

--------------------------------------------------------------------------------------------------------------------------------

原文内容如下：

Jul 06 2011 Published by MarkCC under Programming

As I alluded to in my last post (which I will be correcting shortly), I no longer work for Google. I still haven't decided quite where I'm going to wind up - I've got a couple of excellent offers to choose between. But in the interim, since I'm not technically employed by anyone, I thought I'd do a bit of writing about some professional things that are interesting, but that might have caused tension with coworkers or management.

Google is a really cool company. And they've done some really amazing things - both outside the company, where users can see it, and inside the company. There are a couple of things about the inside that aren't confidential, but which also haven't been discussed all that widely on the outside. That's what I want to talk about.

The biggest thing that makes Google's code so good is simple: code review. That's not specific to Google - it's widely recognized as a good idea, and a lot of people do it. But I've never seen another large company where it was such a universal. At Google, no code, for any product, for any project, gets checked in until it gets a positive review.

Everyone should do this. And I don't just mean informally: this should really be a universal rule of serious software development. Not just product code - everything. It's not that much work, and it makes a huge difference.

What do you get out of code review?

There's the obvious: having a second set of eyes look over code before it gets checked in catches bugs. This is the most widely cited, widely recognized benefit of code review. But in my experience, it's theleast valuable one. People do find bugs in code review. But the overwhelming majority of bugs that are caught in code review are, frankly, trivial bugs which would have taken the author a couple of minutes to find. The bugs that actually take time to find don't get caught in review.

The biggest advantage of code review is purely social. If you're programming and youknow that your coworkers are going to look at your code, you program differently. You'll write code that's neater, better documented, and better organized -- because you'llknow that people who's opinions you care about will be looking at your code. Without review, you know that people will look at code eventually. But because it's not immediate, it doesn't have the same sense of urgency, and it doesn't have the same feeling of personal judgement.

There's one more big benefit. Code reviews spread knowledge. In a lot of development groups, each person has a core component that they're responsible for, and each person is very focused on their own component. As long as their coworkers components don't break their code, they don't look at it. The effect of this is that for each component, only one person hasany familiarity with the code. If that person takes time off or - god forbid - leaves the company,no one knows anything about it. With code review, you have at least two people who are familiar with code - the author, and the reviewer. The reviewer doesn't know as much about the code as the author - but they're familiar with the design and the structure of it, which is incredibly valuable.

Of course, nothing is every completely simple. From my experience, it takes some time before you get good at reviewing code. There are some pitfalls that I've seen that cause a lot of trouble - and since they come up particularly frequently among inexperienced reviewers, they give people trying code reviews a bad experience, and so become a major barrier to adopting code review as a practice.

The biggest rule is that the point of code review is to find problems in code before it gets committed - what you're looking for iscorrectness. The most common mistake in code review - the mistake thateveryone makes when they're new to it - is judging code by whether it's what the reviewer would have written.

Given a problem, there are usually a dozen different ways to solve it. Andgiven a solution, there's a million ways to render it as code. As a reviewer, your job isn't to make sure that the code iswhat you would have written - because it won't be. Your job as a reviewer of a piece of code is to make sure that the codeas written by its author is correct. When this rule gets broken, you end up with hard feelings and frustration all around - which isn't a good thing.

The thing is, this is such a thoroughly natural mistake to make. If you're a programmer, when you look at a problem, you cansee a solution - and you think of what you've seen asthe solution. But it isn't - and to be a good reviewer, youneed to get that.

The second major pitfall of review is that people feel obligated to saysomething. You know that the author spent a lot of time and effort working on the code - shouldn't you saysomething?

No, you shouldn't.

There is never anything wrong with just saying "Yup, looks good". If you constantly go hunting to try to findsomething to criticize, then all that you accomplish is to wreck your own credibility. When you repeatedly make things to criticize just to find something to say, then the people who's code you review will learn that when you say something, that you're just saying it to fill the silence. Your comments won't be taken seriously.

Third is speed. You shouldn't rush through a code review - but also, you need to do itpromptly. Your coworkers arewaiting for you. If you and your coworkers aren't willing to take the time to get reviews done, and done quickly, then people are going to get frustrated, and code review is just going to cause frustration. It may seem like it's an interruption to drop things to do a review. It shouldn't be. You don't need to drop everything the moment someone asks you to do a review. But within a couple of hours, youwill take a break from what you're doing - to get a drink, to go to the bathroom, to talk a walk. When you get back from that, you can do the review and get it done. If you do, then no one will every be left hanging for a long time waiting on you.

Stem form: here...