Here is my list of common mistakes from code reviews and ideas of how to improve. This isn't comprehensive; just a few things I often run into. While I've seen plenty of ineffective code reviews, I still feel strongly about their potential value. Real issues are found even in problematic code reviews. There are many different practices that are helpful for ensuring code quality and I still recommend trying code reviews where possible.

Including Managers

Including managers in a peer review is probably the most common mistake I've seen. The downsides aren't obvious and sometimes including management just feels like the right thing to do. Having managers in code reviews is very tempting for many reasons. One common concern managers have is trying to keep track of how a project is going, what work is in progress, and how well their engineers are performing. Code reviews sound like a great way to accomplish that. Also, code reviews can bring out different team problems and personal conflicts so it is understandable for managers to want to be involved in order to monitor and deal with them.

Theses reasons are legitimate and understandable but they also make code reviews much less effective. There are plenty of other ways managers can look at team performance and the system source code. As the primary focus of a review should be finding bugs, anything that gets in the way of that needs to be avoided. Code reviews can be expensive as it includes several other engineers to look over each other's work. It is important to make them as useful as possible.

Having managers in a review meeting transitions it from a honest evaluation to a situation where making a good impression matters. It could be said that engineers should be professional and shouldn't think like that, but I don't think that is fair. Human behavior is surprisingly predictable. We often behave different in front of authority or someone important than how we behave otherwise. Just like how professionals don't air their dirty laundry in important meetings or in public, engineers are careful about what they say in front of management in review meetings as well.

It can be daunting to go through code review. The author of the code likely spent days or weeks working to get things right and everyone else is trying to pick it apart. If this is done in front of his / her manager, this can create a perilous situation. Even if everyone has the best intentions, presenting flaws in someone's work in front of their manager can go over very badly.

So one thing I've seen myself do in code reviews is tell the author about defects outside of the review meeting. This was when I found a simple / dumb mistake that I thought would look bad. This isn't good process because it should be a team effort to look over defects found and analyze them. But reviews should be about the positive effort of making the work better and I didn't want embarrassment to be part of the process.

Another problem that can happen in review meetings is to nit-pick issues or try to make things look worse than they really are. This too is unfortunately common human behavior when people aren't getting along. Sometimes having a manger involved in the process increases the likelihood of this happening; now appearances matter. So if an engineer thinks another engineer is doing a bad job, he or she has the strong inclination to make them appear that way in code review in front of management.

Ultimately the end result of engineering is what matters most. The details of getting there sometimes can be ugly. A good engineer should want lots of defects to be found in review. This means they are caught before end users can encounter them. It is hard to keep this focus when you are being watched and evaluated. Sometimes the best engineers make the most mistakes because they work on the hardest and trickiest part of the project. So having management there keeping an eye on things can give the exact wrong impression of the quality of engineering being performed.

Moving Along After Quick Approvals

Ensuring that review feedback comes back from all reviewers is usually a challenge. There is always a lot going on in a project and the last thing someone needs is more work of looking over someone else's tasks. For code reviews to be successful they have to work around people's busy days.

What happens is that the people who look over the code with the least amount of detail and care are also the most responsive to getting their reviews in. In software-driven reviews, this results in a quick "accept" and in meeting reviews this results in the reviewer claiming to be ready but having no real understanding of the code. It is easy to just give up and go ahead with a code review with the few people who have said they looked over it. This turns code reviews into rubber stamps. The slow people are the most valuable to code reviews — wait for them.

This makes things difficult. If reviews wait on people who are slow to come up with feedback, things may never move. I don't think there is any simple solution to this. Sometimes this can be caused by procrastination. But what is worse than procrastination is not taking reviews seriously.

An effective review has everyone who is most suited for finding bugs in the type of code being reviewed present and having spent time in looking for them. Don't give up on this goal even though sometimes allowances have to be made. Don't let code reviews switch to having the least attentive people rubber stamp it. There is little point in these kinds of code reviews.

Omitting Omissions

The usual way to to review code is to compare the new version to the old one in version control. Most tools have a way to pull up a difference showing the lines that were added, removed and changed. Some of the best ones allow for tagging lines with comments to track issues exactly where the are in the code. This usually works great but the routine makes it easy to forget to look for omissions.

A diff viewer only shows you what changed &mdash not what should have changed. I don't know of any good way to determine what should have changed beyond experience with the code or just general software engineering experience. So there is no way to prescribe a fix for this common problem. The best I can say is to look out for it. Try to get into the mindset when reviewing code to look around at unchanged code and try to think about what could be missing.

I've seen plenty of bugs in testing or production due to simple errors of omission. These are things that went through code review, weren't too complicated or involved to find, but were simply missed because they don't show up in a version control compare. Watch out!

Coding Standard Reviews

Different organizations can have different opinions regarding the value of having coding standards. If a project has a coding standard, there needs to be automated tools for checking it. It is a waste of code review time trying to find things that violates the coding standard. These kinds of inconsequential issues distract from finding real problems. In situations where automated standard checking tools aren't available, don't address coding standard problems beyond a single line item that standards compliance needs to be addressed. Log that and move on. Don't waste people's time in trying to compare coding styles against a coding standard. It is a distraction and makes code reviews slow and dreaded.

Including Optional Reviewers

I think there is plenty of room to be flexible on this, but it is good to try to avoid having optional reviewers. One potential benefit of code review is to help other people get up to speed with the code and learn about a project that they haven't been involved in. This means it is tempting to include anyone who may benefit from learning the system. This too can distract from having an effective code review.

One challenge to code reviews is ensuring the reviewers have looked over the code as in-depth as is reasonable. As soon as other people are included in the meeting that are there for other purposes, things can go down the slippery slope where rigor becomes optional. Having people involved that don't know the system can also slow down the process by asking obvious questions that could be dealt with in a one-on-one discussion rather than with the whole team.

Getting Off Track

The overriding theme here is that code reviews are important and anything that distracts from finding defects should be minimized and avoided. It is possible to be too strict and take things too far, but usually the opposite is the problem. It takes care and discipline to have an effective review but the results can make the difference between a failed product and something solid and stable for years to come.