Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I'm writing a code review process document for our team; we've never had a formal process in place although we do do some code review.

I've found lots of articles talking about how important code review is, but I have one question in particular that I haven't found the answer to on the web... is the programmer part of the code review? In other words, should a code review be 2 people sitting together, going over what's in the code, or should it be 1 person looking at another person's code?

Most often, our projects are done by a team of 2 people, so before it is sent to QA, should those 2 people just sit together and go over the entire project, including what each of them have written? Or, should they just both look at each other's code? I can see advantages to each.

Finally, what happens with issues found during code review? Are they noted, and then sent back to the programmer? Or should the reviewer note them and then just fix them? Or is it ever ok to make a change and not even tell the programmer specifically what you found?

share|improve this question

marked as duplicate by gnat, MichaelT, BЈовић, Martijn Pieters, Kilian Foth May 28 '13 at 8:58

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

3  
What's wrong with the large number of books and articles already written about this? What of those have you already read? –  S.Lott Jan 4 '12 at 0:20
add comment

11 Answers

A formal code review could go like this:

  1. Programmer sends material to reviewer
  2. Reviewer goes over material
  3. Programmer and reviewer sit down go through the material. Programmer takes notes. Design discussion are postponed.
  4. The programmer corrects whatever came out of the review.

The programmer should always talk to the reviewers, otherwise way too much information will be lost.

Note that since you are just two people on the team, you can probably get by with something very informal, like walkthrough of the code on the programmers screen.

Also note the pair programming is a great alternative to reviewing.

share|improve this answer
1  
+1 this is better than what i was writing –  Ryathal Jan 3 '12 at 21:48
1  
This may be formal to you... but some people may do more or less and call it a "formal review". It's all opinion. –  Dynamic Jan 4 '12 at 1:01
add comment

The basic process should involve the developer sitting down with his peers and going through his code in some depth. How deep depends on the size of the project, how important it is etc. The reviewers should have access to the code before the meeting so that they can concentrate on discussing the defects rather than reading teh code.

Each meeting shouldn't be too long - an hour or so, so if there's a lot of code split it up into more manageable chunks. It's also perfectly acceptable to just review a proportion of the code, though again this will depend on how important the code is etc.

You can either go through the code online or offline - but no changes should be made during the meeting. Just note the defects either as annotations on the print out or in a separate file.

The developer then goes back and fixes the defects, re-running any tests etc as required. Depending on the number and severity of the defects there may be another review - though this might only be a sample of the code.

share|improve this answer
add comment

The review process I am familiar with goes as follows:

  • The source code is managed using a version control system (e.g. SVN).
  • Any change to the software is tracked by a change request system (e.g. Bugzilla).
  • When a developer checks in some changes, these are attached to the change request.
  • Before the change request can be closed, the change must be reviewed by another developer. This can happen either directly (one developer explains the changes to the reviewer), or through a tool (e.g. http://www.reviewboard.org/) after these have been checked-in already. In the both cases, the reviewer can approve the changes or suggest further modifications.

In both cases (direct review or through a tool), if the reviewer approves the changes, the change request can be set to 'FIXED'. If the reviewer requires further changes, the developer implements them and a new review is needed. It is the original developer who implements these additional changes.

For bigger changes (e.g. implementation of new features) the developer does not explain the change down to the smallest detail: the main ideas and implementation techniques are illustrated on the code. For smaller changes (e.g. bug fixes), there is time to have a more detailed discussion.

share|improve this answer
add comment

In my experience, making a code review a formal physical meeting usually results in way too little code getting reviewed, or the reviews being sub-par. I much prefer a tool like code collaborator, gerrit, or the built-in review features of some version control hosting like github or kiln. That lets you inform as many people as you need or want, then they can review on their own time (I reserve the last hour of the day). The original author then makes, tests, and uploads any corrections and whoever found the defect signs off on the correction. This lets you have a more agile, iterative approach to doing reviews, rather than it being one big event at the "end."

share|improve this answer
    
In addition to poor quality reviews, they can also result in adversarial confrontations and/or blame games, especially in work environments that have already become toxic due to backstabbing HR policies like "rank and yank" –  jfrankcarr Jan 3 '12 at 22:36
add comment

In the company I work for, the process is pretty simple: the person responsible asks for the support of a colleague (informally). The colleage should be someone who knows the releavant functionality (but this is not a must).

We do "conceptual solution reviews" and "implementation" reviews.

In the "implementation" review, the responsible explains the problem (if it is not known yet) and the solution (if it is not known yet) and then goes through the code and internal documentation. This happens on the developer's machine (before any code is committed). During this phase, the reviewer can make any suggestions to improve the code or documentation. It is also common for the responsible to ask feedback on specific implementation issues (i.e. show two implementations of the same function and discuss which one is "better").

Ideally, the responsible should show some tests being performed with the new code (and the reviewer can also suggest other test cases).

This is a simple process, but it often finds small problems.

Sometimes there are two review requests / reviewers (but this is due to the company's organization: the code is divided by specialised teams and sometimes you need to change someone else's code).

share|improve this answer
    
This ultimately the most productive method to follow. Formal review meetings waste a lot of time, as they tend to get bogged down in triviality. The more reviewers in the meeting, the worse it gets. It's the classic case of having too many cooks.... I find code review works best when treated almost like a stand up meeting. You discuss the requirements in brief, the reviewer checks that all of your tests pass, that the test covers the requirements (if any), and offers opinion if anything is glaringly bad at first or second glance. The real measure of success should always be working code. –  S.Robins Feb 3 '12 at 10:51
add comment

What you seem to be talking about is what people call "Pair Programming," sitting together and discussing your code to make sure that it's free of defects. This is always a great idea, and one that is normally encouraged. However, it doesn't rule out the need for a formal review. Formal reviews get everyone on your team familiar with the code, plus more eyes is always a plus!

A formal review should always, always include the author so that they are free to make clarifying comments as needed. However, they should never be the lead in the review. This prevents them from having to much control over the process and possibly skipping important aspects of their code.

Many of the other answers cover how to track the defects that are encountered during a code review, so I won't re-enumerate them here, but it is always important to take notes during these meetings and report them back to the author. The author should almost always ultimately be responsible for implementing the changes found during the inspection.

At my school, my professors created a video of what a code inspection should look like and what the major roles should be. We all find it pretty cheesy, but it does cover some good points. You can watch it here: http://www.se.rit.edu/~kurt/videotest/

share|improve this answer
2  
+1 for the video. Worth a watch. –  Thomas Owens Jan 3 '12 at 22:25
    
Love the video. One question about the coding style thing... if I'm reviewing code where someone has named an important variable "x", or perhaps a method is named one thing, but actually does something different (because the functionality has changed over time perhaps), should that not be noted in the review? Is it really defects only? –  GendoIkari Jan 3 '12 at 22:27
4  
Those should definitely be noted in a review. Personally, I would classify those things as defects because over time they could cause problems. –  Daniel Joseph Jan 3 '12 at 22:37
add comment

My view on a code review process.

First, there are two basic rules:

  1. no one can commit unreviewed code
  2. no one can review a change which she contributed to

I. When a colleague is about to commit a change we sit together and

  • check that every test case is green on his computer
  • check that she wrote/modified test cases
  • check that his code matches the existing coding guideline
  • check his changes and focusing on Jeff Atwood’s Top 25 list
  • we go through her changes and discuss it

This is a quick check, can be done in 5 minutes. It is a good way to get a fast feedback, learn about the new change and nothing gets broken

II. The senior developer/team leader should cherry pick from the commits on a daily basis and thoroughly review them and share his thoughts and findings with the team (those shall implement the findings who wrote the wrong code)

III. during retrospective (if you have any) check each other's code for learning

I blogged about the topic here and here.

share|improve this answer
2  
no one can commit unreviewed code - you have an awful process at your company. –  BЈовић Jan 4 '12 at 6:57
1  
Why do you think that? It was not a company policy, it was a result of a team discussion, because we wanted the feedback as soon as possible and we also wanted to know what the others are doing. –  Zsolt Jan 4 '12 at 8:41
    
You either have too many junior programmers (with long way before they learn), or your process is too complex, or you do not have enough work to do. The way you described, it sounds like pair programming, but it is not because another developer only checks the code. In 5 minutes you can not really do a proper code review. –  BЈовић Jan 4 '12 at 8:55
    
I agree that 5 minutes is not enough, that's why the senior dev checks the commits offline afterwards as you mentioned in another answer. We wanted to do knowledge sharing, but we couldn't do pair programming - although it would have been the ultimate solution - and this was the best we were unable to come up with at that time. Additionally, we observed that this approach reduced the number of minor (annoying) defects in the system. I've just checked our data and with this approach we had only 1 minor defect in a medium sized user story while without it we had 5. –  Zsolt Jan 4 '12 at 9:02
    
You could improve your process even further by using static code analysis tools, writing scripts to do some checks, and doing the real code review (off course not every file). –  BЈовић Jan 4 '12 at 9:29
show 1 more comment

I agree with pretty much everything @Karl Bielefeldt said but I'll add one point which I think is important:

Code review is (or should be) asynchronous pairs programming.

The asynchronous part is obviously only true if the coder and the reviewer are not in the same room at the same time. To me, this is best enabled by a tool such as github's pull request. There are certainly other ways of automating the process, and I've seen a variety of them work across domains from medical devices to open source; however, what separates them is the wide variance in efficiency. I don't know anybody who isn't concerned about the time commitment involved these days; therefore, I think it's best to automate the process from the outset.

Github's pull request gets my vote because it's discoverable and easily approachable by beginners via the web interface, and yet, also handles more advanced users who prefer the command line.

The other thing that I like about this system is that it does a good job of separating review metadata (comments about code) from the code itself and focuses the reviewer on providing patches which increase the rate of acceptance.

If you think about it, the economics of code review are something like: quality derived from review is a function of iteration: reviewer's attitude and aptitude plus coder's openness and responsiveness. Anything you can do to increase the ease and rate at which these review iterations take place yields better quality and a more cohesive experience for the team.

I'd also see How should code reviews be Carried Out?

share|improve this answer
    
+1 for the linked post –  sq33G Jan 12 '12 at 15:36
add comment

The simplest way to do a code review is to get a tool (I used codestriker). Then the process is :

  • the programmer picks the code for the review and reviewers
  • reviewers comment the code
  • programmer goes through the comments and accepts whats appropriate

This way, reviewers can do it when they find time for it. And programmer do not need to take notes.


An alternative to code review is pair programming. Some people call it code review on steroids, but it is much more then that.

share|improve this answer
add comment

Formal Code Reviews are a complete waste of time.

They should be done ad hoc while you are building the software amongst the teams working on it.

As you are going along, you refactor your code appropriately as needed. Every code review I've had has been a joke. At Microsoft, things were done while doing. You would get razzed if your code sucked and then it would be refactored by your team as you go along.

At little dive parlors that I worked at before in my early college years, they had code reviews and it was a bit like watching pretentious monkeys who had opinions of grandeur. It was almost like watching a debate over Alien vs. Predator or Catholic vs. Protestant.

It has no real value in my experience and it just detracts time away from ping pong or pool or video games at work which are more effective than formal code reviews.

share|improve this answer
add comment

You're essentially asking two questions:

  1. Should the author of the work being inspected be part of the review?
  2. Should you track the feedback?

In my experience, including the author of the work is essential.

  • The author will have the opportunity to participate in and learn from any discussion that happens.
  • The author is likely best suited to address any defects found in the work; if they only receive an action item list without being present for the review, they miss context of any discussion.
  • The author may be called upon to defend the way in which an artifact was written.

As for tracking the feedback of the review: Yes, it should be tracked.

  • Of the items found, which are training issues?
  • Which are best practices that should be followed for all future work?
  • Which defects were defects because the code is difficult to read, poorly documented, etc.?
  • Which defects speak to larger issues that require more work than a response to the inspection -- should they better be tracked as distinct bug tickets to be worked as resources allow?

Do you have plans to achieve CMMI compliance of any sort?

  • You'll want to track all defects that were found and the fact that each has been resolved.

Does your customer or contract require all code be reviewed?

  • You'll want to track issues and inspection materials so you can answer "YES" and demonstrate that if necessary.
share|improve this answer
add comment

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