Code Reviews – Getting Started

Code Reviews are a divisive topic for many software developers despite their many benefits: They help catch bugs, encourage high quality and consistency of the code, and also promote knowledge-sharing throughout your team. So why is it, that despite the obvious advantages that engineers often push back on having code reviews as part of their daily process? In the end it all comes down to communication (something I will likely say a lot on this blog).

Before dropping the code review hammer onto your team I highly recommend sitting down with them to get everybody’s thoughts and opinions on the value of doing code reviews. There will likely be a large variety of ideas and opinions expressed, but there will be themes that stand out:

  • Code reviews are a waste of time
  • They cause friction among teammates
  • Thorough testing is cheaper and better at catching bugs
  • If we’re crunching I don’t want to have to wait for somebody else to double-check my code
  • I’m a senior engineer – I don’t need anybody reviewing what I write because I am perfectt!

Your team will likely have some combination of those concerns which is normal and to be expected. Some of the concerns are easier to address than others, but I’ll do my best to address each of them here based on my own personal experience. (If you have your own experiences to share I’d love to hear them in the comments below!)

Waste of Time

It only takes one good solid example to disprove this one. For me, my best example is taken from a previous job where I was doing a code review and noticed that one of the key-value pairs used for sending analytics to the server had a duplicate key. This code wasn’t in my domain, but I found the duplicated key to be suspicious so I raised that question in Swarm (our code review software at the time). The developer found that it was indeed a bug and fixed it prior to submitting his final change. Okay, great …and? So think about what would have happened had this bug not been caught. In the best case scenario the bug gets caught during in-house testing by Tracy. Tracy writes up the bug, complete with repro steps, and submits it. The bug goes through a manual triage process and gets assigned back to the original developer. The developer shelves their current work and uses various debugging techniques to track down and fix the bug. And that’s the best case scenario! The cost goes up exponentially if this bug were to go live. Either way, when you add up all of that extra time that could have been avoided by catching that bug earlier it is clear that code reviews are not a waste of time.

Team Friction

This issue is often most easily handled by having a lead developer write up a proposal for code review guidelines. These guidelines should outline the process for performing code reviews:

  • Participant roles and the responsibility of those roles
  • What constitutes a must-fix bug versus a strong suggestion versus a nitpicky comment
  • Procedure for doing code reviews in relation to code submission to answer questions such as: How many people need to approve the review? What types of issues should block submission if found? Is a code review an absolute requirement for code submission or are their exceptions?

Once these guidelines are written up, every other engineer should have a chance to make suggestions for changes, additions, etc. before the document becomes official. These guidelines are now the bible that every developer buys into so when issues arise you can just point to the guidelines.

Now this doesn’t solve every single conflict, but it does help a lot. Most other conflicts happen because the person whose code is being reviewed is in an abnormally bad mood of some kind. Those instances need to be taken case by case as there’s no single solution. Just be patient – sometimes letting a little time pass helps. Other times, you may even consider changing the types of things you comment on. Maybe you add some more comments like “oh hey, this is cool!” Regardless, expect a bit of conflict, but if the team is all bought into the same guidelines then things go much smoother.

Testing Is Better

No it’s not. Move on. But seriously, see my example from “Waste of Time”.

What About Crunch?

First of all, you shouldn’t be crunching. If you are, it’s probably because you didn’t spend enough time doing code reviews earlier in the project. Secondly, if you are crunching and people are tired and they want to check in faster and faster because the deadline is coming up and we are running out of money and the world is going to end if this bug goesintothegameandISUDDENLYHATEPIZZAANDIJUSTGOTTACHECKTHISIN!!!!!!!! Stop, take a deep breath, and stick to your process. Moving faster will not get you to your deadline any sooner and it sure won’t get you the quality you’re looking for.

Where Do We Go From Here?

I can’t help but think of that quote from “The Matrix”:

Where we go from here is a choice I leave to you.

The One, 1999

But really it’s true – where you go from here is really up to you and your team. I personally found that keeping a flexible, yet well-documented code review process document was the key to having a very successful code review process. As a team, we constantly made changes, amendments, and clarifications as time went on and we had one of the most coherent code bases that I’ve ever worked in. … and to date one of the most coherent team of developers I’ve ever had the pleasure of working with.

Advertisements