What's the deal with code reviews?

Question

I've been wondering about code reviews recently, based on how many questions I've seen on here talking about them.

Between my internships, contracts, and full time jobs, I've worked for six different companies, and only one of them has done code reviews.

At the last two places I've worked, code was good (or at least good enough) if all the unit tests passed, and it got the stamp of approval from our tester. We were all compotent programmers, so if the code worked, we were happy.

Now, there were a handful of times where something came up months or years down the line that might have been caught in a review, but they were so few and far between that I'm not sure it'd be useful.

I guess my question is:

1. Do you consider reviews to be useful?
2. How often do you do them?
3. How much of your time is spent reviewing code?

Answer 1

1. Absolutely. There are lots of bugs you can catch early via code review. And there are certain types of bugs you can hardly catch any way else than code review (e.g. some concurrency bugs). But what is even more important: (group) code reviews are a great way to spread and share a coherent, consistent view of design, architecture, coding idioms and best practices throughout the development team. The only better way I can imagine is pair programming - which in fact is an extreme form of code review.
   The prime goal of (classic, formal) code reviews is in fact not catching bugs already present in the code, but preventing (future) bugs from getting into the code, via the above mentioned continuous communication / mentoring / discussion between team members, generated during review meetings.
   This is probably one of the reasons why code reviews are still rare. They show their real benefit only in the long term, so it takes perseverance to get there, and most managers (and developers alike) have never experienced it.

2. In our current project, we have been doing it only occasionally up to now, but it is bound to change. The new aim is to get every code change reviewed by someone else before it gets into production. In an earlier project, we did that and it was useful. In an even earlier project, we did formal group code reviews, and it was still useful, but fairly slow and time consuming. We couldn't have reviewed every code change in that manner without slowing down our pace of development enormously. But for some types of projects, even that pays off. If human lives are at stake, your prime motivation isn't saving dimes.

3. Up to now it was no more than an hour per week on average. According to our new process, this is surely going to increase. In the earlier projects mentioned, IIRC it was 2-4 hours per week on average.

Answer 2

Let's think about the best case scenario: the code is perfect (which will never happen)

Would code reviews be useless? No, everyone involved in the review would learn something and the review would facilitate conversation and exchange of ideas.

Anything less than the best case scenario has been addressed by other answers.

As far as how much time should be spent reviewing and how the review should be structured...this is highly dependent on many departmental/organizational factors and cannot be answered without this context.

Answer 3

You don't mention whether your coders got the code right on the first try, or if they commited code where QA found bugs that required fixing. If QA is finding bugs, perhaps doing code reviews would lower the number of bugs they find. If QA is finding less of the more obvious bugs, they have more time to find the less-obvious bugs. And the more time they spend finding the less-obvious bugs, the less likely your customers will find those same bugs.

Answer 4

1. Yes. Much can be learned from reviewing code, even if it's correct and simply written poorly.
2. As often as time allows, usually daily. At least a few times per week is enough.
3. Usually 30 minutes to an hour, depending on the nature of the review.

Remember, just because every company doesn't conduct code reviews doesn't mean it's a waste of time to do so. Perhaps they don't all simply realize the benefits or even know to include them as part of their development process.

Answer 5

1 - I do consider reviews useful, even if it's an informal process. Unit tests passing doesn't mean you caught all the cases, and unit tests attempt to test code in isolation, so you might have written code that ignored something that may cause problems when your code is integrated with other components. Plus, it might help you improve your ability and style if people can suggest better ways to write what you have written.

2 - I do them if I'm the reviewer for other people's fixes, though in my last company, we used Review Board and I would be requested frequently to look at people's code. I also sometimes glance at major code changes to see if the changes are sane.

3 - Perhaps 5% of my total time.

I always try to get someone to look at my code. As competent as you may be, it's good to have other people you trust to look at your code and see if they can offer up suggestions or give you the thumbs up.

And here's the link to Review Board, which can hook in with source control systems like svn and git.

Answer 6

In some shops pair-programming has taken the place of code reviews. I suppose you can think of pair programming as a process of continuous code review.

In the humanities there is a famous saying that "Even Homer nods". Meaning that even if you are the worlds greatest coder, sometimes you are going to be distracted, off your game, or just plain screwing up. A second set of eyes is incredibly helpful in catching stuff like this. The earlier bugs or 'mis-features' are caught, the cheaper they are to fix. Sometimes these mistakes will pass existing tests, but be laying like booby-traps for when the code needs to be extended.

It also has the side effect of keeping everybody in the team at least a little aware of how the other bits work. This help avoid disasters where the one person who knew how the accounts receivable code worked is hit by a bus or leaves the team.

Answer 7

I consider code reviews very useful and helpful. Even if by some miracle the code is bug free it opens up opportunities to valuable insight. I like to see how to do things differently and/or sometimes better. When I do reviews I may also pick up something new or better while looking at the code someone else wrote.

How often they are done depends on what we are changing. If it is a bugfix then it is likely it will go straight to QA after the developer submits a merge request. If it is something larger and time permits then the code will be reviewed.

The amount of time spent reviewing depends on the code and the developer. Some developers are more adept than others and some code changes are bigger than others so it varies. It never goes over an hour for me...plus the meeting will have about it (~30 minutes).