19

We're currently modifying out development process and I'm wondering if we should try to keep a 100% of our commits peer reviewed.

What is your experience regarding code reviews?

  • Do you tend to spend "a lot" of time on them (say 1/2 hours per day), or just skim over for 5/10 minutes max?
  • Do you have a fixed amount of time to spend per day/week/sprint/project?
  • Most importantly do you think that the target should be for 100% of the code to be peer reviewed or that 100% is not necessary?
2
  • Try to touch 80% of the code with 20% of the effort. Invest in best automated tools that money can buy.
    – Job
    Oct 3, 2011 at 17:32
  • 2
    Automated tools are great, but turn useless unless you have the time and resource to maintain all the tests as up-to-date. Nov 1, 2011 at 20:44

12 Answers 12

19

We have a 'Code Review' task in each story. Someone ideally not involved in the development of that story will review all code changes associated with that story. It works well.

A lot of time? Not very much, depends on how much code - we're looking for obvious errors, typos, basic logic sanity checking, uncaught exceptions, etc.

It's a quality step that does find bugs, therefore it has some value. Allocating time may not be the best way of doing it - how about if something is fairly complex, it should be code-reviewed?

By the way, it's important that someone else does the code review..

6
  • 3
    "By the way, it's important that someone else does the code review..", does the question imply that the same person that writes the code should review it ? If it does where ? I would like to fix that :)
    – Simeon
    Oct 3, 2011 at 11:30
  • 4
    No it doesn't, I was being comprehensive and saying it's important Oct 3, 2011 at 11:30
  • 5
    @Simeon its actually a scarily common misconception that the owner can review their own code. It undermines the whole operation Oct 3, 2011 at 12:32
  • 5
    @TomSquires: Exactly. When you've been working with a piece of code for a long time, you can become "blind" to otherwise obvious flaws in it, because you see it as what it's supposed to be instead of what it is. These problems will be easier to spot for someone who has never seen the code before. Writers have the same problem, and just like books don't get released without proofreading, code should not be released without review. There are also other benefits to doing code reviews, for example it's good for transferring knowledge between the members of your team.
    – hammar
    Oct 3, 2011 at 14:39
  • @hammar - of course trying to find someone who didn't write the code who has the time to become so intimately familiar with it that they can helpfully review it, is a challenge Nov 1, 2011 at 20:38
12

An issue more important that how much over your code is covered by reviews, is how effective are the reviews. If your reviews discover few or no issues, then reaching full coverage will be useless.

First work on making your reviews more effect, then decide on the coverage.

Reviews should be performed not only on code, but also on design.



Also, reviews are no replacement for tests and tools:

  • Reviews can dry run code
  • Reviews can include code analysis
  • Reviews examine reuse and readability
  • Reviews can examine some aspects of efficiency, however, this does not replace actual measurement of run time and finding of bottle necks
  • There are tools for static code analysis
  • There are tools for testing coding conventions, don't waste review time on this
  • Unit, system and integration tests wet run code
  • Unit, system and integration tests test can be repeated automatically, code reviews are usually one-offs
  • Unit tests should have high code coverage and test both main success scenarios and end conditions, code reviews can only partially do this
  • Integration tests test the ability of units or systems to work together, code review can not replace this
  • System tests test functionality of an entire system, code review can not replace this



Try dedicating a preset amount of time per month (or per sprint) for reviews. Select the code you want to review at the next dedicated slot using a heuristic such as:

  • Code that may contain an unidentified bug that was reported
  • Code with that recently has had bugs identified within it
  • Code with performance issues (bottle necks)
  • Code written by new developers
  • Legacy code that was recently updated by someone that was not previously familiar with it
  • Code in new areas
  • Existing code that you want new developers to learn about
  • Code that solves complex issues
  • Code that was identified as complex by analysis tools

And remember, you are reviewing code (or design or tests) and not authors.



I recommend the following reading materials:

Selective Homeworkless Reviews
Best Kept Secrets of Peer Code Review

5

It depends.

It depends on what your software is doing:

  • If it controls an electronic pacemaker or a space shuttle, then definitely yes.

  • If it is a throwaway prototype, then probably no.

It also depends on how well resourced you are, how experienced your developers are, and what you are looking for in code reviews. (Bear in mind that the average developer reviewing someone else's code is probably going to notice style issues and miss subtle algorithmic bugs ... especially given that code reviewing is something of a chore.)

My advice would be to save your code-review effort for code where correctness is critical and the cost of undetected errors is high.

5

First, you need to answer this question: Why do you review code?

With that answer in hand, you can figure out which code needs to be reviewed.

Some code reviews accomplishes exactly what testing does or would have done. If that is the goal of your reviews, then getting closer to 100% is a good idea if you have little testing. However, letting test tools do this would reduce the need for all of the code to be reviewed.

Most good reviews seem to be focused on sharing knowledge and increasing the capabilities of the developers in the review (either the one who wrote the code or the ones reviewing the code). With this as a primary reason for reviews, making sure to review 100% of the code is probably overkill.

3

In a perfect world, everything would be explicitly read by the author and peer reviewed by at least one other person, from requirements specs to user manuals to the test cases. But reviews, even simple desk checks, take time and cost money. This means that you need to choose what you should review and when you should review it.

I recommend prioritizing things to review, choosing how you want to review them, and trying to review as much as you can with the appropriate level of detail. Prioritiziation could be based on the type of artifact, such as stating that requirements must be reviewed, design and production code should be reviewed, and test cases can be reviewed. Within that, you can also specify that high risk or high value components receive a priority in review, or perhaps a more formal review.

As far as time, it all goes back to how high of a priority the component is. There have been times where I've spent 10-15 minutes reviewing, and other times when multiple people have read the code individually then went into a room to do a more formal inspection process that lasts 30-45 minutes (depending on the size of the module).

In the end, it's a balance between time, cost, scope, and quality. You can't have them all, so you need to optimize where you can.

3

As a suggestion, if you plan to do any reviews at all, have some shared guidelines about the review scope and goal to make sure that reviews don't cause needless frictions between team members.

Coherent teams build better projects. People might loose relationships over nonsense or over perfection requests. There is always that one person who would complain about this or that and bug others just because he is like that...

2

I reserve an hour per day for doing peer reviews, but don't always require it. Our code base is shared among a couple dozen products. Our policy is that a trivial change in code unique to one product is okay to check in without review. More complex one-product changes require a review, but it might be as informal as calling a colleague to your desk to give it a once over. Changes in shared code require a more formal review, including developers on other products. I think our policy strikes a fairly good balance compared to other companies I've worked for.

I spend more time per day on reviews than some of my colleagues with less central roles, but I don't consider it an unreasonable amount of time, because before the review policy I could easily waste more time than that tracking down bugs that a developer on another product introduced.

2
  • Is that an average? Limiting a complex review to an hour seems strange, and if there's not that much to review.. well I can't see how an hour a day would be workable? Oct 3, 2011 at 12:31
  • That's not a limit. I set the time based on how long it took, not the other way around. I can usually get 2 reviews done in an hour. If it takes longer than that, your check ins are too big, you're not getting enough design review before hand, or your code review tools are too cumbersome. Code reviews are a check, not a redesign. Oct 3, 2011 at 13:44
2

We've done 100% reviews for code. It's far cheaper than testing, especially 100% code coverage testing. We don't spend too much time on them, reviewing for more than an hour per day becomes less productive. (30 minutes is not a lot).

As you're zeroing in the process, keep notes. What did you find? What did QA find later? What did your customers find? Why did those bugs escape you?

4
  • 7
    How is reviewing cheaper than automated testing? Assuming you write a test once, review a test once, and have a stable test suite, you should be spending far less time and money executing tests than it takes to perform any kind of review (over the life of the project). Also, aiming for 100% code coverage is a waste of resources, which might be to the reason for the greater time/cost of testing. I also question the 30 minutes in reviews - we might review a module for 30 minutes, but there's the prep time of reading the code initially, understanding its role in the system, and commenting on it.
    – Thomas Owens
    Oct 3, 2011 at 13:16
  • @MSalters' claims are not unheard of, though I too am skeptical with it only taking 30 minutes.. I've only read of one place where it was the case that inspection was more cost effective than automated unit testing and that was NASA. In that case they eventually dropped unit testing altogether because it was cheaper to manually inspect the code. Of course, NASA still had a 12:1 tester:developer ratio, so they were doing a lot of other testing too...
    – Michael
    Oct 4, 2011 at 5:35
  • 2
    @Thomas Owens: unit tests find simple bugs. The expensive bugs are those where multiple units combine in unexpected ways. Another kind of bugs is missed corner cases. A developer that missed a case will not write a unit test fo it, either, but a second set of eyes will spot it.
    – MSalters
    Oct 4, 2011 at 7:13
  • @MSalters That's why you write automated integration and system tests as well as unit tests. Really, the only tests that might need to be performed manually are acceptance tests. And by reviewing the tests upon creation would help to ensure that the most critical cases are tested.
    – Thomas Owens
    Oct 4, 2011 at 9:50
2

Have regular code reviews mostly for team building and sharing ideas on implementation. You can learn a lot from your coworkers this way.

2
  • This only indicates a few benefits. Do you think finding errors is important? If so, how much?
    – JeffO
    Oct 4, 2011 at 0:00
  • Of course finding errors are important but the larger benefit is the long term knowledge gained from the code reviews. Perhaps a bug was created by a bad approach that can be prevented in the future
    – coder
    Oct 4, 2011 at 15:02
2

We require a peer code review for every check-in. If no peer is available we arrange for a post check-in review. The reviewer is noted in the source control check-in comment.

These don't take a lot of time, and since they are done between peers there is no toxic adult-child aspect to them.

2

Code Review is, IMO, needed. You're 99.999...% of the time not always going to be right, so you need to make sure it is correct. Do I have a set time? No. But I do take the time to check over my code. Usually I have a colleague do the same.

1

Code reviews may seem daunting, but they are a valuable tool when conducted properly. They will be your first line of defense against design and implementation mistakes. If you aren't conducting code reviews on every feature you put into place, you should start ASAP.

As for how much time to spend on peer reviews, a good practice is to leave 5-10% of your total estimated development time for conducting and responding to the code review.

We have a whitepaper on conducting effective code reviews that can help get you started on the right foot. It is a step by step guide and discusses common pitfalls you might face and how to resolve them. You can download it from our website.

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