I remember sitting in on code reviews early in my career. You took your code, put it up on a projector, and the architects—shadowy figures in the back of the room—ripped it to shreds. These marathon sessions would take all day and were completely exhausting. They certainly improved the code, but they wreaked havoc with both release schedules and people's psyches! Nowadays, I do code reviews a little differently. Instead of submitting our code to the senior architects, we do peer code reviews. Here's how it works:
- I go away and write some code, including tests for that code.
- I run all the tests and tweak until I'm happy with the work. It functions, it's structured properly, it conforms to all of our style requirements, and it's got a really clever solution to that sorting problem.
- I commit the code to my feature or developer branch. It's not in mainline code yet, and won't ship (this is important!).
- I ask for review. If we're working in Git, then I do it by creating a pull request. If we're using some other source code management system, then I just ask for it based on the commit number, or create a patch.
- Someone else on the teamworking in Git—might be the senior engineer, might be the intern—takes a look at the code. They look for bugs, structural problems, unintentional duplications, or any of the mistakes we developers commonly make.
- I fix any problems brought up in the code review, or talk with the reviewer about why I did it that way. Repeat until we're both happy.
- One of us merges the code and checks it in.
If that's the structure, what on earth are we doing? And what does an intern know about how we do things? That's actually the point. Code reviews aren't about approval—they're about learning. A peer code review provides a forum to spread knowledge around the team, including important things like how this feature works, or what patterns make sense. It also provides a way to cover each other. Every one of us has a set of common mistakes that we make, and those mistakes aren't the same as the mistakes our peers make. Doing a peer code review lets me catch that dumb thing that Jake always does, and let's Jake catch that stupid error I always make, and it does it in a casual, non-accusatory forum. Last, peer code reviews are deliberately done among equals. Anyone can review anyone else's code, from the most junior to the most senior engineer on the team; it eliminates the ego and the defensiveness from code reviews. Sounds great! So, how do we do peer code reviews effectively? Here are some tips:
- Mix up reviewers. Don't split off into set pairs and that always review each other's code. Make an effort to review code in an area you haven't looked at before. Sure, the reviewer will ask a lot of questions, but it will flush out assumptions, and everyone will learn a lot.
- Use a checklist. You can start with a standard checklist involving your coding guidelines (e.g., method length and naming), but over time, feel free to add personal items to the checklist. For example, I work with one team that does a lot of filesystem access, and we added, "check for file existence before use" to the checklist. Most of us had made that mistake at least once!
- Hold them in public. At least some commits will be interesting to more than one team member. If I'm changing a core domain model, for example, then probably half the team or more will want to look at it. And that's welcome—frankly, I'm a bit nervous when I'm making that central a change. Having everyone's comments in Campfire or on the pull request lets us work through it as a team. The reviewers can bounce comments off each other, and the code ends up better.
- It's not necessary to review each checkin, but you should review most of them. This one is a bit controversial; I worked with two teams that were very strict about every checkin. However, as long as we're reviewing most checkins—at least every other one—then sometimes it's okay to skip it. You should skip for a reason, though. Maybe the change is incredibly tiny, like a misspelled string in a comment. Maybe it's a production downtime and you're the only one working to fix it at 2 a.m. If it's a choice between getting the system back up and waiting for a code review, then get the system back up. (Although, in that situation it's best not to work alone; frantic people make dumb mistakes.)
- Every team member must be a reviewer and have their code reviewed. This puts the "peer" in peer code reviews. No one is too busy or too junior to be a reviewer. No one is too senior to have their code reviewed. Even if you're the most junior person on the team, jump in and do a review; the questions you ask will help you learn and may help the coder see a problem, even if you didn't see it.
- It's always okay to ask for another review. Peer can be one or more than one person. If you're working on a component that Bob mostly built, feel free to get a review from Jeff and then to ask Bob to take a quick peek. Everyone sees things slightly differently. More eyes, within reason, doesn't hurt.
- Timeliness counts. On a team of more than five people, no code review request should sit more than half a business day. Peer code reviews can be a real bottleneck if they build up; it's much better to take them as they come in. And in the end, a code review makes a good thing to do when you need a break from a hard problem or when you're transitioning between tasks.
Do you do peer code reviews? What do you look for?