14 Answers
14

If you are working on a project where multiple people will contribute to the code base a standard needs to be established.

At this point, in my experience it is best to designate one person as the "king" of code review if you will and what he/she says goes. In this regard, if one user is not adhering to standards the king will take care of it.

As a dev myself, I review my own code many many times to be readable, sensible and everything else. Usually we use javadoc or similar in given languages we code with and use @author tag to attach ownership to functions, classes etc.

I like code reviews, though they can be a pain. The reason I like them is that they get more eyes on the code and a different perspective. I believe that even with pair programming, code should be reviewed. It's easy enough for two people working on the same code to collectively make the same mistake that a different set of eyes may not miss.

If done as a group with a projector, it really should be reviewed individually before the meeting. Otherwise, it is just an annoying waste of time.

I've only done code reviews via email and in a group. Generally speaking, I don't think they should be done in person. You feel a little more pressure to rush through the code with someone looking over your shoulder. I do believe that a tool designed for code reviewing would be a good asset, as it can help with some of the mundane aspects and it should make it easier to flag problem bits of code then it is via email.

The problem with having one person do all code reviews is that it can be a bottleneck. With well documented and designed coding standards it should not be necessary. Depending on the environment/release-schedule it may be a good idea to always have someone as a standby code reviewer.

I do believe that code ownership is a good idea as this person can make it their priority to understand that code and potentially play a gatekeeper role.

+1 for "If done as a group with a projector, it really should be reviewed individually before the meeting. Otherwise, it is just an annoying waste of time. "
–
AShellyOct 26 '10 at 22:04

1

"If done as a group with a projector it is an annoying waste of time".. There, fixed that for you.
–
gbjbaanbJan 17 '14 at 13:34

When you are doing it in person, it's a different process, it's not you reviewing the other's code while he's looking over your shoulder. It's him explaining line by line what he changed, what his changes does, and why while you listen to him. It puts the pressure on who made the code to explain it, not on you to understand and review it.
–
didibusNov 20 '14 at 0:31

At my company each task is assigned a tester to test the task, and also a code reviewer to review the code.

If your product is already released and you want to make sure you aren't doing anything wrong (such as a handle leak, or memory leak) code reviews are a great thing. I think during the initial development before releasing your product, code reviews may be too much work.

If your team has all senior developers, then peer review is still useful. Everyone makes mistakes sometimes. If your team has some seniors and some juniors, then let the senior developers do the code reviews, but still have code reviews for the senior's code as well.

One important thing about code review is that it can catch errors that we make, but it is not a replacement for testing.

At my work we have a very simple rule: changes must be reviewed by at least one other developer before a merge or a commit to the trunk. In our case this means the other person physically sits with you at your computer and goes through the change list. This is not a perfect system, but it has noticeably improved code quality nonetheless.

If you know that your code is going to be reviewed that forces you to look it over first. Many problems become apparent then. Under our system, you have to explain what you did to the reviewer, which again causes you to notice problems you may have missed before. Also, if something in your code is not immediately clear to the reviewer, that is a good indication that a comment is required. And, of course, the reviewer may find problems too. Furthermore, in addition to looking at the change, the reviewer may also notice problems in the nearby code.

There are two main drawbacks to this system. When the change is trivial, it makes little sense to have it reviewed. However, we absolutely have to stick to the rules, to avoid the slippery slope of declaring changes to be "trivial" when they are not. On the other hand, this is not a very good way to review significant changes to the system or addition of large new components.

We have tried more formal reviews before, when one developer would email code to be reviewed to the rest of the team, and then the whole team would get together and discuss it. This took a lot of everyone's time, and as a result these reviews were few and far between, and only a small percentage of the code base got reviewed. The "one other person reviews changes before commit" has worked much better for us.

Not to argue the pros and cons with pairing, but it is hard to dispute the positive effects of having your code constantly reviewed by (at least) another person. The code is even written and designed by (at least) two programmers -- it can hardly get any better than that. I'm saying "at least" because imo, you should try to switch pairs a lot so everyone gets a shot at working with the code.

At SmartBear we not only make a code review tool, we also use it on a day to day basis. It's an essential part of our development process. We review every change that gets checked in.

I think it's a bad idea to have a single gatekeeper code reviewer for many reasons. That person becomes a bottleneck and they have to do too much code reviewing (just to keep the project moving) to really be effective at it (60-90 minutes at a time is the limit of effectiveness). Code reviews are a great way to share ideas and knowledge. No matter how much of a superstar your gatekeeper is, they can learn from others on the team. Also, having everyone do code reviews also creates a "collective code ownership" environment -- where people feel that they own the quality of the code (it's not just QA or the gatekeeper).

We have a free whitepaper on "Best Practices for Peer Code Review" that has 11 tips for making code reviews effective. Much of this is the same content from the book that John mentioned in a more distilled form.

In my company, we never do formal code reviews before checkins, unless we're modifying some highly critical production and don't have the time to perform our standard QA process.

What we do is that each time we feel like a code review would be useful, we add a "//todo : code review by joe" comment to the modified code. Usually, we select Joe because he's owning the modified code, but if this selection criteria doesn't apply (usually, it does), we just chose someone else randomly. And of course, if Joe happens to be available at that time, we use the good old review-over-the-shoulder method.

As a reviewer, the only thing you have to do is to search periodically the whole code for "//todo : code review by me", review the changes, and check the code back in without the "todo..." comment

If there's an issue, we switch back to traditional communication methods (mail/chat/etc).

This method provides us with the two main qualities we're seeking into a review system :

One of the things I try to do in the code reviews I participate in is after reviewing the code myself, I do static analysis of the code, using tools like Findbugs, PMD, JavaNCCP et al, and look at any problems these tools find within the code to be reviewed. I particularly want to look at anything that has an unusually high levels of complexity, asking them to explain why that level of complexity is necessary, or why the suggested vulnerability isn't important.

Where I currently work we produce hardware devices and the software that interfaces with them that go into critical infrastructure. Consequently we have a very high focus on release quality. We use a variant of Fagan Inspection and have a formal review process. We're ISO 9001 certified, among other certifications.

The critical infrastructure industry is very interested in reliability and repeatability of same. :-)

No excuse. Practice pair programming. Its the best code review possible. Any other mechanism results in blame game. Pair programming induces team spirit and collective ownership. Additionally, you debate you ideas with your pair, fail fast, take corrective action and it is only the pair coded and reviewed code that gets committed into Configuration Management System (CMS). Happy pair programming!

I completely agree. Pair programming ensures that code quality is vetted as it's written. Further, introduce TDD and you'll have tested, quality controlled code being released into a feature branch. Test the features in the branch against separately written QA tests. On pass, merge to master. Clean code, clean master.
–
dooburtApr 28 '14 at 15:31

We find the most effective way to do do code reviews is 1-on-1 using github to show the differences in the branch.

Is one person regarded as the gatekeeper for quality and reviews the code, or do the team own the standard?

Depends on the size of the team - team of 3 will have 1 senior who has the best knowledge of good practices, whereas a team of 12 may have 3 or 4 people who will fulfill that role.

Do you do review code as a team exercise using a projector?

In person. 1 on 1 to be less threatening. Sometimes done in the communal area though for knowledge spread. Depends on the exact situation, people, timetable, etc.

Is it done in person, via email or using a tool?

In person. We use git and github and it has great code review and diff comparison tools to make code review easier.

Do you eschew reviews and use things like pair programming and collective code ownership to ensure code quality?

We try to do both as appropriate. That means that when stuck on a particularly thorny problem, or when working on core infrastructure or when preparing for vacation or leaving the company, pairing may be done to share and transfer knowledge. Most code or code that has anything more than cosmetic changes should be reviewed at the end as well.

I have worked at many companies and seen many processes. My current team handles this the best I have seen so far.

We use an agile development process and as part of that we have gates that each story has to go through. One of those gates is the code review. The story doesn't move to test until the code review is done.

Additionally we store our code at github.com. So after a developer finishes their change, they post the link to the commit(s) on the story.

Then we tag a fellow developer to review. Github has a comments system that allows for someone to comment right on the line of code in question. Github then sends an email to the distro, so sometimes we actually get extra eyes from some of our other teams.

This process has worked very well for us. We do use an agile process tool that makes posting the commits easy as well as facilitating communication.

If an issue is particularly sticky we will sit down and discuss it. This works because it is integral to our process and everyone has buy in on how the process works. We can move tickets back to in progress if the code review results in needed rework and then it can be reviewed again after changes are made, or the reviewer may note on the story that fixing the items is sufficient and doesn't need reviewed again.

If testing kicks something back it goes all the way back to in progress and any further changes are also reviewed.