How to conduct a successful code review?

Question

So I work in a shop that doesn't conduct code reviews. What are some of the best practices out there that actually work?

Answer 1

If no one else code reviews yet then ask people to review YOUR code first and steadily increase how often you ask for it. This creates a non-hostile environment and makes it easy for your other team members to start doing it as well.

I only got that one tip, but it's helped me significantly.

EDIT: I posted a blog about something similar a week or so ago. Maybe it will give some more ideas to some people:

http://aboutjustin.com/blog/?p=100

Answer 2

I tend to go away with the code and review it in my own time. I'll then bring it back to the developer and walk through each change with him and make sure he knows why that's incorrect.

There is no point telling someone that their code needs to change to something else because they will just make that change blindly in the future. Code reviews should be a lesson as much as a review if you want to avoid the same mistake in the future.

Since you work in a shop without code reviews currently I'd make sure you push for them for yourself as much as everyone else. Once you catch a couple of bugs that would otherwise have made it to the testers or to your live environment you can report on the cost/time savings you just made. If you want to make a case to management then the $$ saved is often a great way to prove your point.

Answer 3

After dealing with code reviews that didn't work or were a waste of time, I can say that a bare minimum, the ground rules should be known upfront by all the players. Some frustrating situations I ran into included:

Having code that didn't compile given to me. Having too much code to review. Not having buy-in by management - code reviews were check-a-box exercises.

I did find that often a quick run-through of what was to be reviewed by the author was very helpful in making the review time more productive.

Additionally, having some standards in place helped to make things easier to follow. We followed guidelines for naming, etc. and that definitely helped make the code more readable which made it easier to focus on the task at hand.

Answer 4

In my company, we start by having one other developer review the code. If the two developers can come to an agreement on the code, that's great. Otherwise, the two developers will pull in someone else -- usually a senior developer or the software manager (who in our company has actually developed software).

If the code is known ahead of time to be particularly sensitive or challenging, a larger group of developers will participate in a informal review. (Informal because anyone can speak up at any time. Fortunately, we all work well in a team -- there are no cocky or condescending developers in our shop.)

Answer 5

If the changes are small, review over email or in an informal session in your cube.

If the changes are big, email a review meeting notice with the changes to people on your team, including your lead, a few days in advance. Look into a tool that will make your code easy to read with line numbers and colors for keywords, etc. If people are co-located, make paper copies for everyone in advance so they can mark it up before the meeting.

Never check code into your codebase that has not been reviewed.

The main thing to remember is that you are reviewing this to save yourself headaches later on. Take it seriously, and don't be overly defensive about your code.

Answer 6

IMHO the only way that code reviews become successful is when the developers inolved are willing to accept criticism of their code, or are willing to read the code made by other people regardless of skill level.

Without the above any attempt at reviewing code will only degenerate into personal attacks that will only damage the situation that the project is in.

Answer 7

Code review is a great way to spread knowledge among team members, catch more mistakes before submitting, and especially clarifying and re-evaluating your own code as you explain the changes you've made / your new implementation to your collegue.

As I see it (and based on past experience), volunteering for code review while nice to have, will rarely really push the process forward. This initiative must come from the team lead or above and be given as a guideline, otherwise people tend to grasp them as nuisance. If you are not a team lead, then I suggest you try to convince your team lead regarding the benefits of having a code review before submitting code (I assume you work with some version control software).

Especially in big projects under strict deadlines (such as game companies), having a code review is now mandatory and for a good reason.

Good luck

Answer 8

Don't do code reviews for stuff that a code formatter could do: that's way too easy. Make that one point on your review checklist.

Generally, just sit down and have the reviewee show you the code, and since you're smart you'll know when to ask questions. Ask why he made certain decisions, and choose about 20% of them to fix before check-in. Reduce the frequency of reviews as the dev gets more confident.

Answer 9

smartbearsoftware.com publishes a free book on the subject.

Answer 10

We've had good luck in the past with doing peer reviews at commit time. It seems to work particularly good on new teams, or when an existing team brings on new members.

Basically the way it works is this. Prior to doing a commit, the developer calls someone else from the team over to his workstation. He/she walks the person through the code, showing the unit tests, how the code works, etc. Once the review is done, the reviewer "signs" the change by putting their name as a commit comment.

If there is ever a problem with the build both are held accountable. Most teams, once doing this for a while tend to relax a bit and only do reviewed commits when checking in complex changes.

Answer 11

I've worked in places that both did and didn't do code reviews. I also worked on many different teams which did code reviews quite differently.

In my current team, we do code reviews for all but the most trivial submissions. Here's what we do (and people seem to like it):

First, all our code reviews are done before the code is entered into source control. This prevents people from postponing reviews to "later".

Second, we let the reviewer ask questions and interrupt at anytime during the review. A code review is not a simple presentation of your code, it is a validation that you haven't let anything slip through without noticing.

Third, people have to keep their egos outside during the review. Your code might be difficult to read in which case it should be cleaned up.

Fourth, always pick a reviewer that can understand and improve the code being reviewed. A review will be mostly useless if the reviewer isn't knowledgeable enough.

Fifth, everybody does reviews. While still taking into account #4, there isn't one person who must review everything. This is done so code reviews do not become bottlenecks.

I find that reviewed code is of better quality and, more importantly, that more people know about a particular piece of code when at least two people have seen every line.

Answer 12

The two best practices I can suggest are:

1) Have a solid process for code reviews and make sure everyone gets trained on how & why they are conducted.

2) Have a good moderator to keep the review on topic, and move the meeting along.

Answer 13

• Take the review seriously
• Do the review before committing to source control
• Make the reviewing process as easy as possible by
  • Reviewing early and often
  • Using good tools

In my personal experience, using an online tool is incredibly helpful. You can easily send off code for review, and your reviewer can review at his/her convenience, making inline comments and suggestions. Once you try it, you'll never want to go back ...

Answer 14

Reviews are MUST.

Reviews don't catch all defects. But they catch "most". Good reviewer should have both "technical" and "functional" knowledge blend to have outcome of Review be more effective.

I usually ask developer to sit next with me and modify their code infront of them and explain why I do so. (This avoids unnecessary mail communications, miscommunications etc.)

I see in the project release which currently I'm incharge of ... review helped reduce number of defects reported in System testing by one third compare to previous release.

Though recording review comments in system will help us get "Review Efficiency" metrics, I personally feel, "Quality" time can be spent to "Educate" juniors to be aware of their mistakes which will save everyone's time.

Answer 15

Some of the best advice that I've seen on code reviews is from Rapid Development by Steve McConnell. (The well-known author of classics such as Code Complete.) The book essentially is a summary of a number of best practices, with a chapter on the potential benefits and pitfalls of each one that references relevant research. As you should expect, the quality of the book is exceptional. One of his chapters is on how to do code reviews.

I unfortunately don't have that book in front of me. If I did this response would be better. But as I recall he quotes research from IBM showing that code reviews, when done properly, were found to be the most cost effective way of finding bugs. They don't replace QA, but they sure are a good supplement for it. And that is before you add in benefits in training, code standardization, and the like.

If you can I recommend reading that book. In it he lays out quite well the case to get management and fellow developers on board, and gives a specific procedure on how to actually do the reviews that has proven effective in other organizations. His suggested procedure is a little formal and is not the only effective one out there, but .

If you make up your own procedure, the main pitfall to be aware of is the incredibly strong tendency for the person whose code is being reviewed to get defensive about their code. For instance managers should not be involved in reviews. If managers are present, hear about reviews afterward, or are otherwise involved, then defensive behaviour is guaranteed. That will result in a lot of conflict, and very little benefit.

Answer 16

Alex Martelli gives some good tips in his presentation: Code Reviews for Fun and Profit Paper

Answer 17

There's a small tip in Freedman & Weinberg's Handbook of Walkthroughs, Inspections, and Technical Reviews that I really like. If you want to say something like

• Stupid bastard, this will loop infinitely when the parameter is negative!

try to formulate it as a question:

• What will happen if the parameter is negative?

Questions are less confrontational, helping people to keep their egos out of the debate.

Answer 18

Code reviews can be useful. Depending on the project your peers may already be in your code and you in theirs so a formal review is unnecessary.

The most important thing is you are NOT there to criticize programming style. You are there to VERIFY functionality. Does the code perform the function as called out in the requirements? If you start getting into return values and how many columns used and lines of code per function, it is guaranteed to fail. You might as well start complaining about how they organize the icons on their desktop and see how far that gets you. Despite that rule you have the exception, how do you address reliability and risk of the solution without touching on style?

I agree with some others, code reviews are mostly a management check box which often end up having little or no value added. Do not feel compelled to do code reviews "just because". Have a reason, have a plan, inform everyone of the reason and the plan (we are here to verify requirement x and y and not here to complain about return codes). The parties involved should be given adequate preparation time to review the code before they go to the code review, like a day or two. This code should be considered by the developer to be complete and ready for QA, perhaps even through QA and ready for deployment or delivery.

Answer 19

For the love of $DEITY make sure that people actually read your code and have comments. I've been in design reviews where NO ONE SAYS ANYTHING. It's specifically stated 'Please bring notes/problems/etc'. People show up with nothing, say nothing and mooch off of your project money and probably steal your food if you have any.

Second, PLEASE take notes yourself and be responsible for changes. I'm involved in a project where someone noticed that the traces on a PCB were too small for the current it would be pulling. Everyone agreed and slapped each other on the back for finding it. But no one took responsibility and actually fixed it. Testing comes around and an unforseen situation causes current usage to spike and the traces blow. Testing is set back a week while we figure out why the board doesn't work anymore. When we presented why it went wrong to the project superiors someone said 'Yeah, the traces. We caught that in a design review.'

facepalm

Answer 20

Maybe this is a bit off-topic, but what really helped us to do code reviews were the use of code review tools. We use the open source Review Board. It's so easy to comment code that you comment a lot more.

Answer 21

Stephen Friederichs says to take notes. Better yet have someone else taking notes too. If you are the author you will likely be busy and miss something.

Answer 22

As someone who was first introduced to code review at my current job, Google, here's what helped me accept them

1) Seamless integration into our revision system.

Internally we use p4. Externally we use subversion, git and mercurial. In all cases, to submit code for review it's as simple as

g4 upload changelistname -r [email protected]

or

gcl upldate changelistname -r [email protected]

2) An amazing review UI.

We use rietveld for external reviews and the system it was based on, mondrian, for internal reviews.

http://code.google.com/p/rietveld/

It massively facilitates reviews. After typing one of the commands in set #1 above the reviewer and the submitters are sent a link to the review webpage. The UI there lets the reviewer easily compare the changes with a visual side by side diff and double click any line to add add a comment.

For example here's what the reviewer is sent

http://codereview.chromium.org/164130

They can then go through the code and add comments to individual lines and when they click send, an email will get sent to the coder who can easily respond to individual comments. For example:

http://codereview.chromium.org/164130/diff/1/13#newcode341

3) Useful comments from reviewers.

It sucks when a reviewer just wants you to do things their way. But it rules when they find real bugs, then they tell you better ways, when they point out there are already libraries or function in the code you can and maybe should use instead of rolling your own. It only takes a few comments like that to appreciate code review.

Answer 23

Having a 'code inpection' checklist helped my team to get going with regular code inspections. The list is kept short and often reviewed and updated.

Answer 24

Give the reviewers marching orders of what to look for. Nobody is going to look for "everything", so provide some initial direction. For example, marching orders for an imaginary project:

This project adds the ability for users to search for books based on thickness. In the Advanced Search screen, a new box appears allowing a user to specify book thickness in inches or centimeters.

This code modifies table Titles and creates new table BookThick to support this. The tables now use the Oracle Measurements package to handle conversions between systems of measurement. The data feed loader has been modified to handle this new datapoint.

Please pay special attention to the following:

• Rounding errors when converting between units of measurement
• Performance issues in advanced searching from the added JOIN to BookThick
• Proper display of fractional inches vs. non-fractional centimeters

Without some direction, reviewers are going to look at their favorite axes to grind (foreign keys in place? Arrays named with plural variable names instead of singular? etc) Make the most of your reviewers' time, and increase the chances of getting back useful feedback, by pointing them in the right direction.