How I Do a Code Review

I led a team to do a design review of every base class in a 40-man-year project; something like 200 C++ class definition .h files. If there was code in the corresponding .cpp file, we sometimes reviewed that too, but we performed this review early in the design phase, so there was often only a header file of 100-200 lines.

We agreed as a team to follow good-meeting rules. Every design review meeting required a minimum of 24 hours notice, and the review materials also had to be complete and distributed 24 hours in advance. If the materials were not ready in time, the meeting was rescheduled. Each participant was required to have reviewed the materials in advance, and made notes. At the beginning of the meeting, the facilitator asked if everybody had done their homework. If there were not at least three people who had come prepared, the meeting was terminated and rescheduled. Meetings were held face-to-face in a conference room, were scheduled for 60 minutes, started on time, and were stopped and rescheduled if they ran over. In this way, we ensured that the client (the person whose class was being reviewed) had prepared, and that a high quality discussion would justify the cost of pulling three or more people away from work for an hour.

A facilitator other than the client was selected for each meeting. The facilitator ensured the meeting stayed on topic, took notes on issues discussed, and circulated the notes to the participants after the meeting. After the meeting, the client edited the notes to indicate what action they'd taken on each issue. The notes were archived with the other project files.

The materials prepared for each meeting were the class's .h file listing, printed with line numbers for easy referencing, the class's CRC card (a concise and structured form of design document), and any diagrams or other materials the client considered relevant.

Since I knew that having your code reviewed could be ego-busting, and because I was a relatively experienced C++ developer, I became the client of the first two or three review meetings. One point of these first meetings was to demonstrate that even an experienced developer could benefit from a design review, and the team did indeed find issues in my lovely code that needed addressing.

When the inevitable snarky comments started coming in that first meeting, I reminded the team, "Today it's my turn. Next week, it'll be you. Lets keep the tone professional, because you want it to be professional when it's your turn in the spotlight." This set the tone for all the meetings to follow. I don't believe we had a single incident of hurt feelings over the whole project.

After the first couple of meetings, it became clear that there were standard questions we would ask about every class. I compiled a list of these questions and archived it as a project document. (They read like Cliff Notes for Meyers' Effective C++). Within a couple of weeks, these issues disappeared from classes entering the review process.

The cost of preparation, and the keenness of team questions pushed every developer to produce high quality artifacts for review. This had a corresponding positive impact on the quality of subsequent code. Questions about destroying allocated objects led the whole team to take up the use of smart pointers. The RAII idiom was demonstrated and became commonplace in the code. In this way, the best developers taught good practice to the rest of the team, who were learning C++ as they wrote the code.

Later on, as the project approached code complete, we began to review the code, using the same meeting process. These code reviews captured many defects that had escaped testing. Our intention was to review only the most critical code, nominated by individual developers. By the end of the project, we had covered about 60% of the code with at least one review.

The software for this project achieved all its design goals and delivered high initial quality. During the development, our company decided to do ISO 9000. This project received the highest marks from the auditors over a period of a couple of years. Subjectively, we all agreed that we delivered significantly higher quality work than previous projects.

One important aspect of the review process was the team-oriented, consensual model we adopted. Developers were not required to remedy every issue raised at reviews. There was no sign-off or other coercive process in place. The team came to respect the review process precicely because it removed defects from their code. It also respected their individual judgement, both as coders and as reviewers.

The cost of the reviews was significant. My own personal experience was that I spent several days after I was "finished" with a class, getting it ready for review, getting the documentation complete, printing listings, etc. On balance, I thought the cost was justified. I would use the same process again.