Code review - The meaningless ritual
The problem is… most developers hate code reviews and avoid them like the plague. If they can’t avoid them, then they show up and act their part like in a play at the theater of the absurd. They fight getting asleep during the formal inspections and try to make them pass as fast as possible by not commenting much on anything.
The sad part is, most of the time, they cannot comment because they have no clue how the inspected code works. They were not involved in the design of that code and they didn’t have any time to prepare for the inspection. Out of boredom or maybe to show some level of awareness they sometimes cause long discussions about the spelling mistakes in comments and the names of the variables. This consumes most of the allocated time for the review and as a result in the last few minutes everybody rushes to go through 90% of the code just to avoid another meeting. As a result the most important bugs go unnoticed through the code reviews right into the shipped code.
The supporters of the traditional code reviews (Fagan style) always defend the method and place the guilt on the teams who don’t properly implement code reviews. I don’t buy this but the question remains: if everybody thinks code inspections are useful, then why aren’t they done properly? Is it because properly done code reviews are prohibitively expensive? Let’s see…
The traditional perceived value of Fagan style code reviews, the idea that they have as main purpose finding bugs in the code, is one of the big problems of the methodology. Code inspections or reviews as we know them come from the hardware world. Initially Michael Fagan used them for hardware design reviews. This fact is very significant because uncaught defects in hardware design are pretty expensive.
Also defects in low level software were very expensive at the time. Imagine you write a piece of code that has to be burned on a ROM chip (non erasable). You cannot afford to wait and test/debug your code until after the ROM was destroyed. But this is not the case anymore. Unit tests and running the code in debuggers for success and failure paths are pretty cheap with modern IDEs and testing frameworks.
Done properly code reviews are extremely time consuming (take the conclusions of that link with a grain of salt since they sell tools for code reviews). Today’s programmers are more efficient, due to tools and better languages, then 30 years ago. They can create more code faster. But the amount of code inspected per hour is about the same because they are not smarter than people 30 years ago. Imagine 1000 lines of code that one programmer can write in 1-2 days.
With a traditional code inspection 4 people are required to inspect this code at a rate of 200 LOC/hour after they prepared the code review by reading the code and understanding the design. If we are optimistic the whole thing will require 6-8 hours per developer. This means a total of 24-32 hours spent on inspecting code produced in 8-16 hours. Probably this kind of review can find a lot of bugs in the code but is it worth the price? How many teams get away with estimates including 2-3 hours of inspection for every hour of development?
I don’t doubt there are exceptions to this scenario. There are certain situations where any other way of finding bugs is way too expensive. When we send software on Mars we better inspect every line of that code twice because it is too expensive not to. But in most software shops around the world this is not the case. Code reviews are not done by the book because it is impossible within the constraints of most projects.
As a result of having no chance of doing them properly but being forced by the process to do them, people start to pretend doing them:
- Code reviews become process bureaucracy and they are treated as such by developers
- Since they cannot prepare for code reviews because of lack of time people have no interest in the inspected code
- Everybody is busy with their own assignments and it becomes pretty difficult to find at the same time 3-4 programmers that have the time and the design knowledge to inspect a specific piece of code. The developers that have the time (if any) might have not been involved in the design of that piece of code.
- People take it as a chore sice they are not judged by the quality of the inspection and how many bugs they find in the inspected code. Their are evaluated by the code they produce and code reviews eat in their coding time.
- The lack of enough knowledge about the inspected code leads to a meaningless ritual that concentrates on form not fundamentals. This is why people talk about code formating and comments in code inspections.
- Also this lack of knowledge leads to meaningless metrics collected in the review documents and used by the management to prove the effectiveness of code inspections.
Read page 2: Changing the focus of the code review
Pages: 1 2
The way I’ve found them most useful is to treat them, not as a way for the reviewer to spot errors… but as a way for the _author_ to spot errors.
If you have to go through and explain and re-read every part of your code, it’s a pretty good way to catch your own stupid mistakes, stuff you intended to come back to, and so on.
In my experience, it hardly matters whether the reviewer understands your explanation.
To the author of this piece: You are SO RIGHT!
I have also seen code reviews being used as a political and psychologically bullying device… by bosses, mentors, and even overly-articulate peers… to render into submission, either permanently or for a long time, the code reviewee, esp if the reviewee happens to be young/inexperienced or new to the team.. meaning either s/he is new to the technology/platform, or the application design/arch in question, or both.
/SD
I agree, I think that at best, code reviews should be used to enforce certain company practices and ensure proper use of modern standards. I think most programmers are competent enough to write tests for their own business logic. Making sure that another person who is new to the project can carry on from where you left off is the most important.
1000 LOC in 1-2 days? In a classroom exercise situation, developers write about 30-40 LOC/hr. In a production environment, it’s actually lower than that. Plus, you only get 3-5 hours of coding in a typical work day. That’s more like a couple hundred lines of code in two days. Figure two weeks or more to produce 1000 LOC.
Also, what about time spent in test? Individual code reviews of the developer’s own code + a group walk-through of the code by the writer presenting to a small group 2-4 folks + individual reviews of that code by each of those attending where each focuses on a different “viewpoint” + a wrapup meeting will take a lot of man hours. However, study after study shows that this saves more time in test, than it costs.
It takes discipline. It takes training. It takes the right goals and focus (the ones you pointed out are all good). And, most significantly, it takes conviction by those asked to do it. It’s this last step where organizations most commonly fail. But if you really look at the numbers and make a decision to do it right, reviews, walkthroughs and inspections are a net gain.
Testing, not review, is the way to find errors in the code. That said, though, I’ve always found informal code reviews to be a good way to share information - we use them to educate other members of the engineering about projects that they may not have been directly involved with.
I have been tasked by management to be the “Code Review Nazi” on our team. After coming to the same basic conclusion concerning the ineffectiveness of traditional code reviews, I worked out was a weekly code review, which I explain better at http://m0smith.freeshell.org/blog-software/2008/03/code-buddy-art-of-weekly-code-review.html
The basic idea is that, like builds, code reviews should happen early and often. The less code the review, the more effective it can be. Also, the earlier in the process a review happens, the more useful it will be to the project.
It has taken some doing to get people on board but reviews are now happening much more regularly.
The problem now is how to gather metrics on the reviews. Every measure I have seen actually ends up being counter productive. If you measure the number of bugs found, then you will either have the Wallys who code up a mini-van or people finding bugs where there are none. If anyone has an idea on code review metrics, I would love to hear it.
Great posting, thanks.
There’s a free book on code review at http://codereviewbook.com that agrees with a lot of your conclusions. The last chapter also promotes the author’s product, an online code review tool.
Disclaimer: I work for that company. Doesn’t make the book any less free or the conclusions any less correct though.
In my previous company, I had the privilege of working with an extremely gifted engineer and an exceptional C++ programmer who, as a matter of fact, was informally inspecting/reviewing every perforce submissions of everybody else in the team (six or seven senior engineers, he himself being an architect). That in addition to his own work.
In my experience, the process of code review is in direct correlation with the quality of the people in the team and with the company’s technical culture.
@Liviu
The keywords here are “exceptional” and “informal”. People make processes work not the other way around.
I am a very beginner to the java programming. I found some of the valuable things from above. However, I just want the step by step programming tips. Could you assist me through your blog?
I hope so and look forward…
“I think that at best, code reviews should be used to enforce certain company practices and ensure proper use of modern standards.” - completely agree
I now review units tests rather than the code. If I find sane tests that cover both the core functionality and a good selection of edge cases, I’m happy. The only comments I generally offer are to add additional tests (which hopefully cause the code to improve!)
While I have not done a ton of code reviews, I always found it to be a learning/training experience as opposed to a way to spot errors. We’d very rarely change code after a review, but I think it has tremendously helped the quality of future code because of the “Oh… I didn’t know you could do XYZ”. And not only that it should be the author and the reviewers who get extra info like that. Examples of things I’ve picked up: System.IO.Path.Combine is much better than a string builder. The awesomeness of XmlSerialization instead of XPath (and string builders). The exceptions dialog box. And many many more. Some of these I learned while the author, some while the reviewer.
I enjoyed your article. Basically, don’t blindly start adopting a practice. You have to reason and figure out how it applies to your situation.
For me the code review has been really helpful. I’m a new employee, so I was quickly brought up to speed with policies and design goals from the start, while yet being somewhat productive. It wasn’t so much about finding specific bugs.
I have a similar position on coding standards. If there was a way to count them, then I assume that there could be millions of coding standard documents on the planet. Most of these say the same thing such as how should curly braces be written.
Coding standards should be just plain old common sense. But then again it is the least common of senses