Wednesday, May 1st, 2013

Effective learning through code workshops

By

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.

  • http://twitter.com/PallabDv Pallab Banerjee

    Hello Nicholas,

    First of all excellent article and thanks for the sharing. Though I have a few queries.

    When code has distinctive owners in the team, and team knows who’s code is being reviewed, how do we eliminate the situation of being too critical?

    Have you noticed or foresee any problem with this approach on a team of developers with varied level of expertise?

  • http://twitter.com/slicknet Nicholas C. Zakas

    Hi Pallab,

    Thanks for the feedback. One of the key aspects of this approach is to make sure the reviewer doesn’t mention names during the review. I tend to emphasize that the code is all of our responsibilities, not just one person (after all, if the owner goes on vacation, who will fix it when it breaks?). Also, having a person who didn’t write the code be the one to present it helps to break the defensiveness cycle. The code owner doesn’t have to defend decisions in the code, though it may be useful to explain things at different points in time.

    The overall goal is to learn from the code, the good and the bad. As long as everyone approaches the meeting in that way, the tone tends to stay positive. We all want to get better, this is the way to get better as a team.

    I’ve seen great results in groups with varied experience levels. Generally I’ve seen the biggest improvement in those who the least amount of experience as they are able to soak up additional tips and tricks that otherwise would not have been communicated to them.

  • brianm101

    Basically you are not doing code reviews or at least not the way we see it! – Its more code a case of code assassination (and author).

    Code review is more a case of someone being talked through the code by the writer, the ‘reviewer’ is basically looking for errors or missing functionality or perhaps something amiss with the big picture – hey you know we have library routine to do that or that needs to be virtual otherwise it will break. Its a sanity check of what’s been done. Code quality, layout, comments is done by the code writers manager – and that’s something different (although could be the same if a small team)
    You certainly don’t ever ever ever ever do it front of a group of people – ever!!!!!

    • http://twitter.com/slicknet Nicholas C. Zakas

      I must disagree. Perhaps you’ve had experiences that have led you feel that way, and that’s exactly why these code workshops are designed in this way. The whole thing is setup to be non-confrontational and positive. No one feels attacked and the reviewer is more of a discussion leader than anything else. We’ve still uncovered a lot of issues that we wouldn’t have found otherwise, and the great thing is that no one’s feelings have gotten hurt in the process.

      • brianm101

        Maybe you have been lucky, but not all engineers can deal with this type of environment of code review, guess it might work by natural selection, those that don’t like it leave!

  • Ralph D Wilson II

    I have to agree with Pallab, an excellent article.

    One of my first memories of a team I joined early in my career is of the Code Review process and another memory from that same team is our Design Review process, which preceeded the Code Review. (For the same reason that you chose to refer to your process as a “workshop”, these were both referred to as “walk-throughs”.) Contrary to most experiences with those titles, these were amazingly productive and educational experiences. Why? Because the team had a range of experience levels and these reviews were used as an opportunity to educate those of lesser experience while at the same time including them and their questions/observations in the process.

    Those reviews consisted of the developer/author of the design/code presenting the design/code to the team. The idea was for the author to take the rest of the team through the design/code, explaining along the way the decision points and conditions that would be handled as well as how the design/program would handle anything other than valid data. The expectation was that the author would point out how the design/code met the requirements during the walk-through and that ALL of the requirements would be addressed during the walk-through.

    Once the author had finished the walk-through, others on the team would begin to ask questions, mostly along the line of “How will this design/program handle the following data/situation?” Sometimes, questions would be aised along the lines of, “At [some point in the design/code], was there a reason that you chose that approach over [some other approach]?” However, the tone of the questions were never accusatory and, instead, were inquisitory. Often, the questions kicked off some very interesting discussions of the relative merits of various approachs to solving issues.
    Unfortunately, since that time, I have never had that same kind of experience, no matter what the design/code review was called. ;-)

  • Tarang Patel

    A very good article. I liked the idea of Code Workshop.

    As for the Pallab’s question I think there should be a Code Review session before Code Workshop between the Reviewer and the Code Owner where the owner can argue/comment or suggest if needed. This way developers can agree upon one best solution. Then the Code Workshop can be more of a learning session than just one person opinion.

    What do you think ?

    • http://twitter.com/slicknet Nicholas C. Zakas

      Code workshops definitely augment regular code reviews, though I don’t think they need to be tied together. Allowing people to express their opinions is good. The point of having other people in the room is so that it fosters discussion. It’s not one person dictating to everyone else, it’s one person leading a discussion with others in the room.

  • James Forde

    Get yourself CodeCollaborator from SmartBear and make code reviews a joy to behold!!!

  • Jim L.

    I think it is a great idea, but 20 people in a meeting? How long should the code workshop meeting be
    scheduled for? 2 hours? 4 hours? With 20 people chiming in, giving their take
    on the code, it could take a long time to review. Also, how large a chunk of code (how many
    lines) should be reviewed? Do the engineers
    have enough time in a day to sit in a meeting and review the code?

    Possibly, using a code inspection rather than a review (or
    workshop) may be best when you have this many participants. And, using good tools to conduct the
    review/inspection makes life much easier.
    I agree with James Forde about
    using CodeCollaborator, since the place where I work uses it. (It does have its drawbacks, but overall, it
    is useful.)

    There has been much research done on code reviews and
    inspections, especially in the early days (60s and 70s) by IBM and it is
    available on the web. I also did my
    master’s thesis on code reviews where the library research on past studies was
    quite eye-opening.

    In terms of production and business needs, possibly
    reserving the code workshops for the more difficult pieces of code (or design)
    may be of benefit to all, since not only is there a review done, but also, some
    learning will be achieved.

    • http://twitter.com/slicknet Nicholas C. Zakas

      The code workshop is just one hour, two 3-minute blocks. 20 people is the max, there’s usually fewer than that. The point is to give people constraints such that they need to decide what the most important aspects to discuss should be.

      As for if engineers have time, that’s an organizational issue. If the organization cares about code quality, then you schedule this into your process. An hour a week isn’t too much to ask to improve the overall code quality.

  • http://www.opwernby.com/ Dan Sutton

    As a programmer with decades of experience in the field, I can state categorically that the best way to achieve massive increases in code quality is through the judicious and constant use of abuse, denigration and making programmers feel stupid. They learn quickly. Their reward is that it stops happening to them, and they get to do it to others: as Ogden Nash wisely pointed out: “Big fleas have little fleas upon their backs to bite ‘em. And little fleas have smaller fleas: so on ad infinitum.”

    As predators, programmers respond well to the kill-or-be-killed mentality inherent in this approach: political correctness, concern for the feelings of others, and perfunctory respect have no place in a development environment. On no account should a programmer ever feel completely comfortable: if this should happen, disaster will strike: they mutate from agile cheetahs to sedentary male lions… of course, this is a circular process: just watch how cheetahs taunt lions on the plains of the Serengeti.

  • kplcjl

    Thanks Nicholas, all too often, I have seen code review in the form of an E-mail sent to the group, no-one is responsible for the review and either the code is ignored or panned. With 20 people sending e-mails, I’ve got to do my own work and read 500 to 1000 lines of unrelated code (to what I’m doing or each other.) I like the idea of being responsible for one piece of code and as well as my own work, I like the idea of accepting responsibility to need to spend time on their code as well and being acknowledged for the effort I put into the review as well as producing code.

    I have to admit, Dan’s work environment seems familiar too.

  • JoyG

    In our company we use reviewboard integrated with perforce and git. I feel its an excellent way to have ongoing reviews and tracking changes.

  • Erwan Jegouzo

    While I agree with you on this approach, I don’t think that this is enough to ensure a good code base throughout an entire project. If a team of 10 developers are working on a project, then doing a code workshop once a week would only a tiny fraction of the entire code base. The majority of the code will never be reviewed.