Another one of my never-ending flow of initiatives from the past year or so has been encouraging and promoting peer code reviews. This started as an extremely bare-bones process. Before committing a change to source control, a developer would grab a peer and simply walk them through their changes. My thought here was to get a second set of eyes on the code to catch obvious problems and force the developer to review their changes to avoid silly mistakes (e.g., Oops, I left my email address hard-coded in there!)
The next phase of this agenda would be to introduce a more formal code review. Grab a conference room and a peer or two, and walk them through the solution. As a reviewer, it’s hard to understand what’s going on by looking at a single file in a single changeset. You get no context about what’s trying to be accomplished. I can tell you if you miss a null check, but I’m less likely to notice if you’ve violated design principles without knowing much about the bigger picture. I’m still working on how to make this part of the process work, but that’s not what I’m writing about today.
I was watching a webcast about peer review, and it opened my eyes to a huge flaw with both of the processes introduced above: they occur after development is complete. We all know that the earlier a problem is identified, the less costly it is to fix. By waiting until the end of development to do a peer review, we’re ensuring that problems identified by peers will be as costly as possible. Peer reviews can also identify optimizations that could save development time. Refactor that function into a common place so that each class isn’t re-implementing it the same way. Boom, you just saved all the time that would’ve been spent re-implementing that function in subsequent classes. Peer reviews often result in a list of items that need refactoring. If you wait until the end, you’ll have a bigger list that takes more time that is ultimately less likely to be completed.
In addition to the time-saving opportunities, there’s also a human element that comes into play. When I think I’m done with a project, the last thing I want to hear from my peers is everything that’s wrong with what I came up with. In that scenario, I’m more likely to be resistant and defensive to suggestions. You think I should take all this stuff out of those TWENTY classes and move it into a base class? We’re deploying next week–there’s no way we can do that!
Additionally, the presenter in the webcast, Karl Wiegers, pointed out that peer review isn’t–and shouldn’t be–limited to code. The earlier you catch an error, the less costly it is to fix. So why not spend time peer reviewing the requirements document? A missed requirement that comes up late in the development process is usually a mess. Utilizing peer review on the requirements document might save some headaches later. As a bonus, you also introduce the requirements writing process to your peers, potentially adding depth to the team. This helps strengthen the team’s bench because even junior members can become familiar with the process of defining requirements before it becomes necessary for them to do so. The team also benefits from being more familiar with what’s in the pipe, which makes starting new development less of a telephone game.
The final message that I want to leave you with is exactly what the title suggests: peer review early and often. Review requirements documents and they’ll become more consistent, better defined, and knowledge will be shared between team members. Peer review code before it’s done to identify optimizations that can save development time and reduce the need for refactoring later. Peer review designs and test plans to improve quality and grow the team.
adamprescott.net 