Do you feel code reviews are excessively time consuming, distracting, and generally just more work than they're worth? Perhaps your process is to blame.
I've worked on a lot of projects that have done code reviews poorly. Perhaps you have too. I've experienced first-hand having my productivity destroyed with constant peer review interruptions. I've felt the discomfort of trying, as diplomatically as possible, to tell a defensive co-worker their code sucks, only to have them check it in anyway. I've been in the position of avoiding that argumentative team member to perform reviews so my quick check-in doesn't turn into an all-day discussion. Sound familiar?
I've also worked on projects that have done code reviews right, and that have reaped the benefits. I've seen junior team members get up to speed fast while learning advanced techniques they wouldn't have otherwise learned for years. I've seen every member of a team learn to actually use the same conventions (e.g. fewer foreach's more LINQ). Most importantly I've seen teams with a variety of skill levels consistently and methodically increase the quality of large code bases as a team. It is possible.
After a team has completed development on an iteration they must complete a code review on all new code developed during that iteration. This step is a part of the team's iteration closeout procedures. The entire team is given time to review changes individually (typically comparing against a tag from the last code review) and then compile a list of issues which they bring to a group review.
In the group review everyone goes through the individual issues and compiles a finalized list of code review issues, clarifying, removing, or adding as necessary. Team members sign up for code review items, items are estimated (often aggregated by person because they tend to be so small), and for the most part are put in the next iteration.
Those are my code review experiences. What are yours? Please feel free to post in the comments.
I've worked on a lot of projects that have done code reviews poorly. Perhaps you have too. I've experienced first-hand having my productivity destroyed with constant peer review interruptions. I've felt the discomfort of trying, as diplomatically as possible, to tell a defensive co-worker their code sucks, only to have them check it in anyway. I've been in the position of avoiding that argumentative team member to perform reviews so my quick check-in doesn't turn into an all-day discussion. Sound familiar?
I've also worked on projects that have done code reviews right, and that have reaped the benefits. I've seen junior team members get up to speed fast while learning advanced techniques they wouldn't have otherwise learned for years. I've seen every member of a team learn to actually use the same conventions (e.g. fewer foreach's more LINQ). Most importantly I've seen teams with a variety of skill levels consistently and methodically increase the quality of large code bases as a team. It is possible.
A Process That Works
The best process I've seen so far works like this:
After a team has completed development on an iteration they must complete a code review on all new code developed during that iteration. This step is a part of the team's iteration closeout procedures. The entire team is given time to review changes individually (typically comparing against a tag from the last code review) and then compile a list of issues which they bring to a group review.
In the group review everyone goes through the individual issues and compiles a finalized list of code review issues, clarifying, removing, or adding as necessary. Team members sign up for code review items, items are estimated (often aggregated by person because they tend to be so small), and for the most part are put in the next iteration.
Performing the code review in this manner has the following benefits:
- Interruptions, a developer’s primary productivity killer, are kept to an absolute minimum
- Architectural and cross cutting issues are easier to spot than with per-feature code reviews
- Placing code reviews as part of iteration closeout procedures ensures they are performed and maintains quality as a top priority
- New team members (in particular) benefit from consistently reading code, and seeing first-hand conventions and techniques employed by more senior members
- Pre compiling a list of issues eliminates all-day marathon type code reviews
- Unlike with a per-feature review it is not immediately apparent who is to blame for any bad code, reducing negative feelings and maintaining a focus on what’s important: the code
- Expensive refactorings can be reviewed by the Scrum Master (or customer or whoever) and postponed as priorities dictate
Summary
Code reviews don't have to be awful. It really is possible to increase quality and bring everyone's game up a level. If you've had negative feelings with code reviews in the past, maybe it was the process. Maybe it's time to try again.Those are my code review experiences. What are yours? Please feel free to post in the comments.
Comments
Unfortunately I haven't had the opportunity to be on a project where the entire team paired. Pairing seems to be a hard sell in my circles, unless it's specifically for training a new person (which I've done for a few iterations, and code reviews were still necessary).