How do you promote a culture of peer code review with your team?

Question

We have tried many methods such as

• Code review required before check-in (hard to enforce)
• Monthly code review sessions (tend to side-track, time consuming, too high level)
• One or two devs with free cycles review changesets as they are added (low participation)

We use TFS and could write a tool to leverage the API or build a workflow that might help promote peer code review.

What have you found to be successful within your teams?

I'm not above gimmicks and bribery and prefer carrot over stick.

Answer 1

What have you found to be successful within your teams? First off:

Track the number of defects you have with each release. Make sure they know what doesn't work in production. This isn't to assign blame, but you have to get them on the same page. They have to realize with a little bit of work, they can reduce this number.

Now:

The only thing that is really beneficial is that you have to show them why it is important. Once they get that, they won't want to live without it. The key to getting them to adopt it is to let them set it up on their terms. Tell them that you think it would be a good idea and get their suggestions as how to implement it. If you try and tell them how it is going to be, chances are all you are going to do is aggravate them, and they'll resist the adoption of it. They have to go through a learning phase of trying it and figuring out how to get it to work. Once they "own" the process (think it is their own), they'll start to enforce it, because it was their decision.

Answer 2

I can't find the link anymore, but it was discussing how peer review of code changes could end up concentrating on pulling the existing code base rather than the changes themselves apart - not very productive when you consider what the purpose of the review is. Instead the article recommended doing a diff on the current file and the previous version and only reviewing those changes. Sorry I can't attribute the article to it's relevant author, but it has stuck in my mind.

BTW, to be clear there is nothing wrong with always looking at the code base as a whole and getting everyones feed back / making changes. However, that is not the purpose of a peer review of code changes.

Answer 3

We have adopted a few things that help code reviews to be more beneficial.

• We made it clear that reviews are not personal and not a blame situation. Need to encourage conversation, even if it is not what someone wants to hear.
• Reasonably informal. Too much process can be its death.
• Allow reviewers to do it on their time. Pass out info, and give a date to finish.
• We have a FAQ for common review type questions which help getting everyone speaking the same language.
• Have real resource time allocated for reviews. Should not be treated as an afterthought, but more as a step.
• Keep the reviews small/focused and do frequently. Don't make it a special event. These should be happening daily/weekly.
• Also, don't have too many reviewers. This takes a lot longer to manage. Obviously include reviewers that are familiar with the design of the code (maybe they were involved in earlier high level and/or detailed design reviews?).

Many more, but a good list to start with!

Related thread : http://stackoverflow.com/questions/465104/what-is-the-best-way-to-encourage-communication-in-a-team

Answer 4

When you are looking to do something with the process, you can change the workflow of your work items to include the "In code review" state. (this could be an optional or a required state)

You then have at least of list of all items to be code reviewed.

You can also have a look at the TeamReview project on codeplex: http://teamreview.codeplex.com/

Answer 5

Just as one data point, the software consulting firm I work for makes peer code review part of the normal development process. Every patch is sent, in a standard format, to the project mailing list for review. Patches can only be committed if someone approves it.

I think this practice should be the norm everywhere.

Answer 6

The following worked well for my teams.

1. Every piece of code was reviewed before going into 'the build'.
2. Reviews were a mentoring process (mentioned in other answers), not adversarial. Deviations from that were treated as education opportunities.
3. We had a review system linked to source control so that non compliance could be monitored (nicely).
4. The review system allowed us to do stats on reviewers as well as reviewees.
5. One reviewer per review, as often a peer as a 'senior' person.
6. Clear coding standards & expectations.
7. Review diffs not the whole code - avoids picking apart existing code.
8. The code author was expected to review their stuff first and that was rigidly enforced. So although we encouraged a mentoring process for the most part, if the coder just tossed garbage over the wall which wouldn't build, we tossed it back with a ticking off.
9. We reviewed everything: Built the code, checked coding standards, sample checked unit tests to see that they had been run and walked through the requirements too.
10. Comments were recorded in a database but we insisted that we talk through the review rather than just leave comments to be picked up - and often missinterpreted - later. Use the phone + chat for remote developers.

Sounds expensive? Actually it paid back many times over in lower maintenance costs.

Impossible to implement in an old team? No. We implemented this from no code review at all, set expectations and just kept at it until it was the bedrock of our process. That built a team which was just excellent, which had a fantastically low defect rate and absorbed new people well (usually). When that team broke up it was the saddest day of my career.

Answer 7

Some things I look for:

1. Make sure to choose a reasonable subsection of your group who are open to new ideas to do the review.
2. The people who review shouldn't show possessive or evangelical coding beliefs.
3. Comments of the reviewer should be reviewed by another reviewer.

The first is mentioned as it helps to keep debate going in the right direction. It also helps the spread of new ideas and/or insights into older problems. I'd have never have learned of the for_each in C++ if it weren't for someone suggestion.

The second prevents people who are prone to be overly territorial or "there is only one right way, mine" from causing code reviews to be confrontational or antagonistic. Sometimes "better" to one person is "another way of doing it" to another and the people who can't differentiate don't belong in a code review.

The final makes sure that code reviewers focus on the important things instead of the non-important. A sure fire way to see that someone doesn't take the time is to see only punctuation comments about comments in the code. Or listing of spelling mistakes. Or harping on 3 vs 4 spaces of indentation.