73

I've never found the ideal way to perform code reviews and yet often my customers require them. Each customer seems to do them in a different way and I've never felt satisfied in any of them.

What has been the most effective way for you to perform code reviews?

For example:

  • Is one person regarded as the gatekeeper for quality and reviews the code, or do the team own the standard?
  • Do you do review code as a team exercise using a projector?
  • Is it done in person, via email or using a tool?
  • Do you eschew reviews and use things like pair programming and collective code ownership to ensure code quality?
1

13 Answers 13

32

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 better name, a comment, or a refactoring 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.

1
  • Very useful, thanks! I'm preparing my own team' sessions and I think this could be a good approach.
    – Neonigma
    May 24, 2018 at 15:11
14

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.

3
  • +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. "
    – AShelly
    Oct 26, 2010 at 22:04
  • 1
    "If done as a group with a projector it is an annoying waste of time".. There, fixed that for you.
    – gbjbaanb
    Jan 17, 2014 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.
    – Didier A.
    Nov 20, 2014 at 0:31
7

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.

2
  • 2
    Link down...........
    – Pacerier
    Feb 22, 2015 at 11:25
  • Sorry about the link rot. I edited to the current URL, but that won't prevent it from happening again. Feb 22, 2015 at 20:11
3

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!

2
  • 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.
    – dooburt
    Apr 28, 2014 at 15:31
  • 1
    Pair programming does not work for a very large percentage of software developers and I'd venture to say that it likely rules out the best developers. Most SW developers are into computer programming because they are introverted, meaning they prefer to work with computers more so than people. I for one, need to get into "the zone" to be effective and that means "don't bother me". Leave me in my zone and I'll do the work of 4 or 5 other developers and then some.
    – Dunk
    Apr 6, 2017 at 15:04
2

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.

If a function is not correct we talk to owner, same with class etc.

2

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.

2

I recommend using code reviews if you are not doing pair programming.

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.

2

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 :

  • very light overhead
  • traceability
2

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.

YMMV

2

At my company we have compulsory code reviews for junior programmers, and voluntary reviews for seniors. There is no designated code reviewer, review requests get sent to all developers.

This system works well - reviews are done as time permits and changes can be reviewed by several sets of eyeballs.

The excellent and free Review Board tool works really well for us, and has proved to be an excellent way of communicating reviews.

1
  • 3
    voluntary reviews for seniors. I've worked on multiple projects where senior programmers definitely could use code reviews...
    – Michel
    Nov 7, 2016 at 14:13
1

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. :-)

1

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.

As with all coding items, the right answer should take into account:

  • Type of company (e.g. startup vs. large corporation)
  • Company Size
  • Number of developers
  • Budget
  • Timeframe
  • Workload
  • Complexity of code
  • Ability of reviewer(s)
  • Availability of reviewer(s)
0

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.

Not the answer you're looking for? Browse other questions tagged or ask your own question.