Code review – The meaningless ritual
Now, from what I wrote up to this point, you might think I don’t find any value in code inspections/reviews. And you are right to think so, I don’t find value in code inspections done as I just described them. On the other hand I think code reviews done “properly” can have a huge value. But by properly I don’t mean the traditional Fagan style definition. Keeping around processes that cannot ever work because of permanent constraints and that are against human nature is a form of self delusion many companies are guilty of.
I think everything changes if we redefine the real value of code inspections:
- Verify how and if the original agreed upon design was captured in implementation. This is in my view the most important reason for having code inspections. I’ve seen many cases where the implementation didn’t resemble at all the original design, sometimes with good reasons but many times with no reason at all.
- Spread intimate knowledge of the design and implementation in the team. This is one of the most effective ways of reducing the overall number of bugs in the code. Educated people make less mistakes.
- Teach established coding style and practices to new team members. When people know what is expected from them they just do it. This will reduce in time the overall volume of code inspections.
- Keep the focus on why things are implemented the way they are.
- Finding bugs in the code is a bonus but not the main focus.
This change of focus, from finding bugs to making sure the implementation solves the right problem in the right way, will achieve, in the long term, a better quality and maintainability of the shipped code.
The developers will start to feel these code reviews actually help them write better code and learn more things.
In order to help people focus on the items above a number of practices can be used by a conscious team:
- Write good comments – not what the code does but why the code does what it does.
- Use code formatting tools to enforce the agreed upon standard. IDEs like Eclipse can do this with a touch of a button.
- Use spell checkers for comments.
- Run all the code through the debugger before calling a code review and white box test both the success and the failure paths. This will for sure unravel more bugs than any code inspection.
- Decide case by case what code and whose code will be inspected and for what purpose. Some code is inspected to teach the author coding style. Some code is inspected because it is very complicated and cannot be tested properly in a debugger (multi threaded code). Not all the code is inspected.
- The author leads the review in the natural design dictated order. This is because he is the one who knows best both the design and the implementation.
- The reviewers decide the pace. The author cannot go to the next file/concept untill all the reviewers understand the current topic and agree to move on.
- Start with a design walk through and with a description of the main problems encoutered at implementation (maybe they caused design changes). It often happens that the author discovers problems in his code while trying to explain his decisions to others.
- Differentiate the review format/focus based on code level and type. The structure is more important for high level code while the detailed implementation more important for algorithms.
- Differentiate the review format/focus on code purpose – new vs. modified vs. bug fixes.
- Use automated regression tests to signal side effects caused by code changes.
- Make sure programmers have ownership and authority over areas of code, not only responsibility. This will lead to a desire to be trusted in the team. This trust system can be used to reduce the number of inspections and to focus the inspections on the code that needs it most.
I believe better quality in software comes from better educated developers working in functional gelled teams. People make processes work, not the other way around. Good people can make almost any process work, but if they are allowed they can come up with better processes. I didn’t talk about tools designed to help code reviews because, while some of them can be useful, they cannot replace reason and motivation.
I would call this approach Agile Code Reviews but I don’t want to use the term Agile since nowadays everything is Agile. Since RUP was described as Agile I cannot wait for the day when I can read a paper on how Fagan style code inspections are and have always been Agile.
Read page 1: Code review – The meaningless ritual
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
Very interesting article, very nicely written.
The thoughts of the author are true in a lot of companies, I’m afraid ;-( Unfortunately !
But it doesn’t have to be like that. If the team members themselfs are of the opinion that Code reviews do make sense, than code review have an added value : Young team members can learn from senior staff, product/requirement managers with ICT background can check if the business rules are correctly coded, etc …
But, again, I’m afraid that the situation described by Daniel Pietraru, where code reviews are blindly enforced is widespread. No surprise that in that case code reviews become meaningless rituals …
http://www.message_dronmonal.com/
Thanks for sharing.The best one that, I am very interested to join this.
New promotion on http://www.epicleveling.com due to summer holiday.
Fast WoW Power Leveling Level(1-30) – 70
1. Leveling your character from Lvl(1-30) to Lvl 70;
2. 1000 WoW Gold;
3. Epic Flying Mount & 225 Riding Skill;
4. First Aid to 375 Points;
5. Class Quests and uncovered Flight Paths.
Original Price: $79.99
Preferential Price : $ 69.99 Time : 7-8 Days
Cheap WoW Power Leveling Lvl 70-80
1. Leveling your Lvl 70/70+ character to Lvl 80;
2. 2000 Gold or (1000 Gold + 225 Riding skill);
3. Epic Flying Mount & Normal Flying Mount;
4. First Aid to 450 Points;
5. Cold Weather Flying Skill(77 lvl+);
6. Class Quests and uncovered Flight Paths.
Original Price: $59.99
Preferential Price : $ 56.99 Time : 3-4 Days
Maybe your char reaches a higher lvl without professions,gears and so on. to obtain them, just take a try at http://www.epicleveling.com/World-of-Warcraft-US,Honor-Power-Leveling,1,2.aspx,http://www.epicleveling.com/World-of-Warcraft-US,Profession-Power-Leveling,1,3.aspx and http://www.epicleveling.com/World-of-Warcraft-US,Gold-Item-Power-Leveling,1,4.aspx. Hateful Gladiator Armor Set Package Price : $ 99.99 Time : 5-7 Days,Mining / Herbalism / Skinning 1 – 450 Points Price : $ 54.99 Time : 1-2 Days,Paladin Epic Mount Quest Price : $ 59.99 Time : 1-3 days
Sorry dude but I am not agree with you on Code review, at least it provides advantage of four eye check which at 90% checks some silly human error and if done with some more enthusiasm it can point out some serious design, performance, logical and functionality error which would be hard to find during testing, debugging or unit testing. so to me its an integral part of Java development.
Javin
How Java Classpath works behind the scene