How to Do Effective Peer Code Reviews

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?

Comments

  1. BY RobS says:

    I wish we’d always done more code reviews.
    Most recently, our reviews were more about style and standards. We were looking to see the styles used by others so we could get a sense of how others do things and see if there were certain techniques that we could pull from them
    And the other part was to continuously review our standards to see if there was a reason to change and, if so, have a meeting about the proposed changes.

    I think we need to do 2 different kinds of reviews.
    First, understand that when we hire programmers, we assume they know how to program and therefore can write solid code that compiles and does the job.
    So review type 1 is see if there are certain techniques being used that others might have better ways to handle the task, including things like organization and scalability. This would likely also include exploring possible design problems in the code (due, maybe, to bad specs, so we could help get everyone better specs next time)
    Review type 2 is exploring the code for standards like self-documenting code and naming conventions.
    Both of these are related to learning, but one focuses on quality and the other focuses on getting the team synchronized with doing things a common way.

    Further, after reading your article, it occurred to me that initially the senior developers should offer their code for review to newcomers so that they can: (1) allow the new guys/girls to learn the team’s techniques and (2) allow them to offer new ideas that may have been missed in an ever stagnating environment. As the team gets more used to each other, code reviews should get quicker and quicker as all you seek are design flaws, at least until the next new developer is hired.

  2. BY Mike says:

    “Peer review” like other methodologies is probably not applicable to every IT group/request/department. I’ve worked in environments which used junior programmers as testers; that was interesting. Something about egos and a room not large enough to hold the senior programmers. In a different place, I designed it, coded it, tested it and released it. In yet a different place the end users tested what I designed (with their considerable input) and then I released it when they said “OK”.

  3. BY Alessandro Z says:

    Great post. We do our peer reviews using CodeCollaborator, used to use Juniper plug-in for Eclipse. Most of the comments that come in are style and documentation. I’d like those comments to be lesser and more about potential design flaws and easier way of doing things as I am a junior developer and want to learn and grow. However, the review process here starts from the design phase, so before code is even written, design is has to be reviewed. Many problems are caught here. Thanks for the article.

Post a Comment

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>