Guidelines for Code Review in OpenStack (and other projects)


Bad behavior will label you forever. Bad code can be fixed.

My team and I have a semi-montly book club, where this month we read Brené Brown’s Daring Greatly, and the discussion focused on vulnerability – specifically the courage it takes to put ones code up for public code review, and how being a code reviewer doesn’t. And yet, the more I think about it, the more I realize that from a social standpoint, being a code reviewer is a far more vulnerable position than that of the contributor. Why? Because the contributor is the one doing the work. The code reviewer is at best a gateway, at worst a troll, and since the nature of OpenStack is to keep historical records on everything, our behavior is etched into the internet.

Thus, in an effort to encourage fewer trolls, I’ve extrapolated a few rules for codereview from all the various communication training I’ve had in the past. Some of these come from the above book, others from Crucial Conversations, yet others from my own personal experience. Feel free to add your own experiences in the comments.

Rule #1: Stay engaged.

I tried to ask my reviewer a question, and it took them a week to get back to me.

Every contributer everywhere.

As soon as you choose to get involved with a code review, you’ve taken on a huge responsibility and time commitment. It includes, but is not limited to: Responding to questions, responding to updates to the code review, checking every revision to make sure your concerns are still met, and providing rationale for your comments.

All of these activities must be done in a timely manner. Part of this is because a code review is a conversation, and the longer you wait, the harder it will be able to pick up where you left off. The other, and perhaps more important part, is because code patches bitrots quickly, and depending on the velocity of your project, the patch may be out of date before you can respond.

My own rule of thumb is: If I can’t respond within a day, I’m not doing my job and should let someone else take over. Personally, I think my own rule of thumb is too long, and an optimal timeframe is less than 2 hours.

If you are the lead, or core, on a project, this rule takes on a whole new meaning, because you are responsible for all patches. While you yourself may not be able to remain engaged, you can collaborate with other cores to make sure that someone else can cover it.

Rule #2: Get out of the way.

I’ve been trying to land this patch, but one of the critical reviewers went on vacation.

Every contributer everywhere.

This is the natural opposite of rule #1, and translates to: Don’t bite off more than you can chew, and tag out gracefully.

It is important to recognize your own limitations. If you know that you’re already doing too much, then it is your responsibility to let someone else handle the review and not engage. More importantly, if you’re already engaged and suddenly find that you no longer have enough time – whether you’re going on vacation or got pulled onto a different project – then it is your responsibility to find someone else to take over your code review. Barring that, you should clearly communicate that you do not have the time, and remove any objections you have to the patch. After all, if it’s important enough for you to engage in the review, then it’s important enough for you to stay engaged.

Rule #3: Grok the code.

My reviewer has asked me to revise my code three times!

Every contributer everywhere.

There is nothing more frustrating than having a reviewer do a casual pass over, ask for some nits, and then come back on a later review with serious architectural concerns. It’s also quite disrespectful to both the contributor, the project, and to your own professional reputation, as it shows that you may only be there to get your two cents in.

Everyone wants to be understood. This is a basic human need, and it is incredibly frustrating to engage with someone who doesn’t seem to care. As a code reviewer, it’s your responsibility to set aside the necessary time to understand what the contributor is trying to accomplish.

A good template for a code review comment would be the following: “I see you did X, and Y, and I agree with both of these, but then you did Z, and I feel that you could better accomplish this in a different way.” This communicates a strong understanding of the contributor’s goals and thought process, and moves the review forward in a constructive way.

If the only comments you have are nits, then you can also say so. Something as simple as “I’ve read this, agree with the goals and the implementation, however the following documentation bits need to be corrected”.

Rule #4: Identify Common Ground.

All I’m trying to do is land this patch, and my reviewer keeps finding ways to deny it.

Every contributer everywhere.

You and the contributor are there for the same reason: To make the project better. Understanding this is critical, because it forces you to realize that you’re both on the same team. You, as a reviewer, care about roadmap and code quality. He, as a contributor, cares about feature/bug support and meeting a release deadline. All four of these are necessary and required parts of quality code, and each of you has an important role to play.

In short, you are colleagues, not opponents. This realization alone will change the entire dynamic of your interaction, and make it less contentious.

Rule #5: Be constructive.

My reviewer asked for a change, but didn’t specify how and/or where to make that change.

Every contributer everywhere.

Contributors cannot read your mind. Contributors may not have as much experience with the codebase as you, either. As a result, it is very easy for a your review comments to gloss over critical items which you assume are common knowledge.

Be explicit: If you ask for a change, specify where that change needs to be made, how it needs to be made, and why it needs to be made. Provide a code example if possible, and provide the contributor a way to contact you for clarification. Be careful though, it is very easy to be patronizing: Don’t mansplain.

Lastly, don’t be afraid to dive into the code yourself. In fact, in many cases it will be far faster, as you will reduce churn, miscommunication, and review lag. It also has the added benefit of marking you as a strong collaborator, someone who’s not above doing the actual work. Remember: Anyone can comment. Only those who care, contribute.

1 Comments

Leave a Reply