Code review process?

Question

I'm looking to formalize the way we do code reviews at within my department because at the moment it't a bit disorganised.

Do you have a formal code review process? If so what is it and how do you think it could be improved?

I'm working in a team of 5 developers which means there is not very much experience floating about the company in this kind of process.

UPDATE To Clarify:

I say I'm working in a 5 man team, typically we are all working on different projects, 5 people is the size of the development department. Which in my experience is tiny (My last job there were 40 of us with up to 6 working on any given project)

Answer 1

A basic code review process is the following:

1. Make a change
2. Let some other dude in your team check the change
3. If approved, commit the change. If not, make a better change and go to step 2.

Works nice on small teams, but big organizations demand more control and meetings. If you want to go extreme with your team, you might as well start pair-programming.

My tip if you want to start small and use a smart tool is that when you're doing version control you could always add an issue tracker on top (such as Trac, redmine, Bugzilla and whatnot). An issue tracker makes reviewing the changes easier to do because you can refer to the changeset or commit to one particular issue/feature. You can ask a fellow developer to look at a registered pending feature and have a look at the changed code (via the changeset) to see if there is anything wrong with it.

Answer 2

When I worked at on launchpad.net I liked the code review process there. It's very highly structured, relies on various ad-hoc instruments, is quite dependent on the use of DVCS, and ties into other agile practices. But I'll try to get the gist of the code-review practice.

Each development task (usually associated to a bug or a specification) is done on a new branch based on trunk. All tasks should produce a unified diff that is at most 400 lines long. If a task produce a change bigger than that, it must be broken down in smaller tasks using refactoring techniques. Of course, exceptions can be granted on occasion.

The 400 lines rule was established because code review time is not linear on the patch size. So small patches are important to preserving velocity.

When a task is complete, the branch is posted on a wiki, where an robot will generate the diff for review. A code reviewer (there is a specific group of code reviewers within the development team) picks the branch, and write an email that looks like a reply to the patch. The mail must include a mark such as "merge-approved" (you can integrate that as is), "merge-conditional" (trivial changes are required before integration, but another review will not be needed), or some other marker I do not remember right now that means "fix what I asked, then we'll have another review round".

All code reviews were recorded on a mailing list, so other reviewers and developers could see them, and so code standards could be more efficiently disseminated. A wiki of best practices and coding standards was also actively maintained.

In my current position, I am starting to make code reviews with collaborators who are not professional programmers (more math oriented people) and have no experience in the programming language (Python) or code reviews. I found that going over the code-review email in pair-programming style helped defuse the feeling of personal attack that is often felt by people who have no former experience with code reviews.

Answer 3

As Spoike said pair-programming would be good in a 5-person team (except for the fact that you have an odd number of people but that's okay). Then everyone will run their eye over the code if you pair swap often enough (maybe start with a swap every new working day).

In the place I'm working at the moment it works on two levels:

(1) when you've finished your task, you will get another developer to pair-review the code (2) after each iteration/sprint (in our case three weeks), we go into a more formal code review where the chief architect and other developers are able to run his eye over all the code before the meeting and leave comments in our code review system. In the meeting would be all the developers, one or two developers from another team along with the chief architect - then we all go through the comments left by everyone and categorise each comment by whether it needs to be fixed before release or not

In my opinion, pair programming and pair swapping is the optimum solution. Not only will everyone have their eyes on the code, it's constantly being reviewed as people communicate and question what is being done. It also means that issues are raised in a much faster manner. You don't have to wait until 3-4 weeks after you start a task that someone else doesn't like what you've done. By then, you may have implemented it totally different and sometimes it may seem too late to change it which is kind of counterproductive. And if you do change it a month after you started it, you would have wasted a chunk of your previous iteration doing it "wrong".

I know people in the company who just go to those meetings who just nod their heads and say 'yes' to everything just to get out of the meetings quicker. Pair programming and swapping if done correctly will give you constant code reviews as well as knowledge sharing and give a forum for discussion and optimum design.

Answer 4

At my company we define the following roles for a code review meeting:

• Moderator - he/she is responsible for making sure that the code review meeting is effective and focused on the goals.
• Author - The developer that wrote the module/code that is being reviewed;
• Reviewer - One or more developers that review the module.

The minimum number of persons for an effective code review is at least 3. The obvious author, a moderator and at least one reviewer. This widens the probabilities of potential bugs being found.

The formal process is the following:

1. Developer creates the code

2. Developer creates a DRR - Document Review Report - where it states the code that is being reviewed.

   • The classes and CVS revisions that are being reviewed;
   • The roles for the code review and the persons assigned to each role;
   • The CVS tag that marks the code being reviewed on the source control system.
   • The CVS tag that will mark the code after modifications resulted from code review are applied by the author.

3. Code review meeting is scheduled by author.

4. Code review meeting is done. The defects found are recorded in the DRR.
5. The author corrects the defects and signs the DRR saying that the modification has been done.
6. The author tags the CVS system with the code review output tag declared in the DRR.
7. The author hands the DRR to the moderator.
8. The moderator checks if the modifications have been done and can be found on the source control system.
9. The moderator signs the DRR saying that the modification has been confirmed.
10. The moderator hands over the DRR to the quality department for archiving. If no quality department is in place the project manager should save a hardcopy of the DRR on the project folder.

---

Example:

Developer John Doe creates a Math class that adds two numbers. He does his best to put some bugs to test the code review process.

public static class MyMath {
    public static int Add(int a, int b) {
        return a - b;
    }
}

He puts his code in the source control system and the class MyMath.cs gets revision 1.1.

Then he goes to write the DRR - Document Review Report.

Modules/Classes

MyMath.cs myproject/implementation/common/MyMath.cs (1.1)

Roles

Moderator - Jill Goner [email protected]
Author - John Doe [email protected]
Reviewer #1 - Matt Groening [email protected]

Tags

Input - PROJECT_0_0_1_INPUT_REVIEW
Output - PROJECT_0_0_2_OUTPUT_REVIEW

John Doe tags the current MyMath.cs revision 1.1 with the tag PROJECT_0_0_1_INPUT_REVIEW on the source control system.

John Doe schedules the meeting.

The meeting takes place and the defects are marked on the DRR. The total count of defects is also included.

John Doe goes and makes the changes on the source code and commit the modified revision to the source control system.

John Doe applies the output review tag PROJECT_0_0_2_OUTPUT_REVIEW.

John Doe signs the DRR and gives it to the moderator Jill Goner.

Jill Goner goes ahead and checks that the modifications were done and that the output tag was created.

Jill Goner signs the DRR and hands it over to the SQA (Software Quality Assurance) engineer.

Aside hint - Keep code reviews under a 2 hour maximum length. Humans tend to oversee important things mostly when reviewing code when the code reviews sessions are lengthy. It is better to split a review among more sessions that try to do it all at once.

Answer 5

I've worked under a process very similar to smink's above. It takes a little getting used to at first, but once everyone gets over the first hurdle, these types of reviews really help.

One thing I'll add; use tools to format and check code for adherence to standards. Once you get everyone's code looking the same, reviews can get past the petty arguments over curly brackets, indenting, comment characters, etc. and actually review the code. We actually made the output from the code checking tool one of the artifacts to review.

Answer 6

1.) Code reviews have to be planned and accomodated in the schedule.

2.) Code Overview/Pre-view is what we do. i.e. author of the code first explains the functionality of the code he wants to be reviewed. Algorithm level. This gives a good idea and background for the reviewer and he gets various perspectives towards the code during the actual code review viz: functionality perspective, optimization(execution time/memory) perspective, portability, readability

3.) At one meeting only 2 hours of code should be reviewed. We found this by trial and error that 2 hr is kind of limiting case(not to less, not to much) keeping the quality of reviews/review comments

4.) Send the code to the reviewers 2-3 days in advance, based on the complexity/LOC of the code, after the pre-view has happened.

5.)Reviewers review it offline and come to the meeting with their comments.

6.)In meeting author listens to each review comment, accepts/rejects it with the associated reason

This process has served me ok till now.

-AD

Answer 7

This is what successfully worked for my team in the past. Combo of face to face and email code reviews.

Background:

Team 10 developers. 1 big project. 1 project/development manager. All based physically in same office. Based upon the principal that no one developer owns their own code. They may be responsible for codin it in the first place but may not be responsible for bug fixing later. The code review process was made easier with visual studio team system. We could shelve our code and someone else could unshelve the code to review it without having to actually commit the code into the development branch.

Code review process:

1. Design: we provided a high level over view of how to solve a problem. No code at this stage. Talk with fellow developer who is available and development manager. Benefits of early design review is getting input that a single developer might have missed. Re-design etc if required.
2. Code it.
3. Submit code/link to code changes to another developer for peer review via email/electronically. Best bet is to submit it to a developer who was not involved with design review. Note that the reviewer should review the code without input from the original developer. This is important. We approach it this way for several reasons. If the code can be understood by someone who has not written it then it is safe to say it is maintanable should the original developer be no longer around to ask (left the company or whatever) or even if they are. If the code reviewer cant understand the code then its given back to the developer and he/she makes it more understandable either by greater documentation or refactoring the code. It could be as simple as adding more information to the bug description that the code change is realted to. This in it self saves so much time in the future. The ability to pick up someone elses code and understand it quickly is very advantageous.
4. Developer and code reviewer get together and discuss code/improvements.
5. Developer makes changes if required.

Code reviews in my opinion and very important and advantageous to the development process. You learn from reading other peoples code. Your skill in reading other peoples code increases. You learn about other areas of a system you are developing. It's another head with knowledge of how something works therefore another resource to code in that area if required. Its spreading the knowledge. It's not easy to sit and listen to someone point out flaws in your code which naturally points people away from code reviews.