Effective learning through code workshops

At Box, we’re very interested in the quality of our code, which is why we’re constantly evaluating our processes to figure out how we can do better. We even have a team of Code Reliability Engineers (CREs) that help others write better code and provide training both internally and externally. Recently, we’ve turned a critical eye towards code reviews and have been implementing a new process called code workshops.

The trouble with code reviews

Code reviews are a tough topic to approach as many have had bad experiences in the past. The first code review I ever personally participated in was, in fact, horrible. It was organized by the senior members of a team I was on and the process was strange. We all sat in a room, spending the first 20 minutes staring at printouts of someone’s code and scribbling notes on them. After that, we went around the room and everyone shared what they thought of the code while the author of the code listened intently. Then, the author was allowed to defend the code. I hated that experience. In my mind, we have just wasted time sitting together and making someone incredibly uncomfortable for no good reason. We didn’t actually learn anything and it was time spent away from being productive. I felt like there was some value to doing code reviews, but this process felt wrong in every way:
  • There was an overall accusatory tone that encouraged people to find things that were wrong and trivialize things that were right.
  • People seeing code for the first time in the review meeting was a complete waste of time. Why weren’t we prepared?
  • The negative comments put the code author on the defensive, which led to less-than-productive conversations.
Over the course of my career, I’ve heard a lot of feedback around these points whenever engineers say they hate code reviews. The kicker is that engineers actually love talking about code. We talk about it on Twitter. We talk about it over email and IM. We talk about it over lunch. We think some people write code better than others. We like semicolons. We hate semicolons. We love talking about this stuff, so why is it that we hate code reviews so much? The problem is typically the code review process. Too many times they are set up as adversarial (as in my first experience), which leads to defensiveness and bad feelings. When processes are too loose, people don’t feel like there’s much value in participating. When processes are too strict, people feel judged. Unfortunately, the best way to learn about code is to discuss it with other people, so how can you create a system that people actually enjoy and learn from?

Building a better code review

Over the years, I’ve worked with a process that I’ve come to call code workshops (since code reviews have a negative connotation, a different phrase eliminates some bias). Code workshops are a group session that are intended to socialize team expectations and best practices, as well as uncover issues and points that need to be addressed by the team as a whole. Code workshops take place on a weekly basis for one hour and with a maximum of 20 people in the room. The session is split in half, with two reviewers each reviewing a file (or set of files, if small enough) for 30 minutes. The key pieces to the meeting are:
  • A reviewer is assigned (or selects) a piece of code to review ahead of time. The reviewer must choose a file that he or she didn’t write. It’s the reviewer’s job to present the work of someone else.
  • The reviewer reviews the entire file, not just diffs. Diffs are okay for bug fixes but to get a deep understanding of the code, you need the extra context. The goal of the reviewer is to learn as much about the code as possible.
  • The reviewer reviews the code before the workshop. I typically recommend people set aside an hour for this. During that time, the reviewer adds comments directly into the file marked with REVIEW.
  • The meeting begins with everyone closing their laptops except for the reviewer and one other person to take notes. The point is to have everyone pay attention to what’s being discussed so we don't waste anyone’s time.
  • In the meeting, each reviewer goes over the file with their comments, focusing on the interesting pieces of code. The code is projected so that everyone can follow along.
  • The reviewer cannot mention people’s names during the review. The point is to review code, not people. Everyone is considered an owner of the code so there is no need to get defensive. If there are problems in the code, then the group needs to get better as a whole.
  • It’s the reviewer’s job to encourage discussion through findings in the code. Look for interesting or problematic patterns, opportunities to establish new conventions, or pieces of the code that need more explanation.
After the meeting, notes are sent out to all of the participants and relevant documentation is updated (if, for example, new conventions emerge).

Code workshops improve code quality

I’ve seen the benefits of this approach firsthand. The most obvious benefit is having a group align their expectations together. Code quality ramps up very quickly when everyone understands what is expected of them and can hold each other responsible for decisions that were made by the group. This is much more effective than sending out a document or wiki page that no one stops to read. Another benefit of this approach is that certain types of bugs become immediately apparent, whereas the same issues can easily be missed by simply reviewing diffs. Looking at the file as a whole means getting a better understanding of what the code is doing (or should be doing). Strange patterns are more easily perceived this way. Perhaps the most important benefit is giving people a forum in which to communicate effectively and safely with one another. It’s my firm belief that most code quality problems can be traced back, in one way or another, to poor communication. When communication improves, and expectations are explicit instead of implicit, that’s when real change happens. We’ve been doing code workshops at Box for almost two months and have seen some excellent improvement in how code is being written. Along with the identification of problems that had been missed for a long time, we also have started to formalize conventions that were previously implied, increased understanding of older parts of the code base, and cleaned up a lot of bugs. As a result, we’ve also added tools to help identify common issues we were seeing (such as adding JSHint for JavaScript). Getting a consensus understanding of what is good and what is bad has made new code less error-prone and more maintainable in the long-run. What’s even more important is that engineers don’t hate the process. It’s an opportunity to do what we love to do, and that’s talk about code with each other. Now that we have a way to express ourselves that is non-confrontational and structured with positive outcomes in mind, we’re free to continue evolving our discipline and making improvements week over week.