What does a code review look like? [duplicate]

Question

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?

Answer 1

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.

Answer 2

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/

Answer 3

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."

Answer 4

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

Answer 5

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?

Answer 6

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.

Answer 7

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.

Answer 8

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.

Answer 9

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.

Answer 10

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.