Ethics for code reviewers

In the previous post, I explored a bit how ephemeral most of the artifacts of software development processes are. One of these processes is code reviewing, which is arguably a major player in code quality, knowledge acquisition, and even team dynamics.

Even being so important, the outcome of the code review process — the review itself — tends to reach a very limited audience and have a short life time. It’ll be hard to change that picture given the nature of reviews: they are conversational, and address specific issues for the integration of a change in the project. At the same time, even if code reviews are not generally useful as permanent documentation, we can increase their value as reference material by improving the quality of those conversations. Having a good conversation has many other great side effects, of course.

As a small step in that direction, what follows are personal guidelines that I have been evolving empirically over the years as a software developer and code reviewer. They may not bring you fortune and fame, and are not always easy to apply, but hopefully they will help improving your experience as a member of your team and the value of those reviews.

Explain why

Unless the change is about an extremely obvious mistake, explain why you’re suggesting it. If the reasoning was natural to the author, he’d have done it in the first place. Good explanations also help avoiding the same mistake over and over again, and are much more rewarding to the listener. They also become a target for future references.

If you don’t have enough time to justify it and would rather provide the review sooner than later, one approach is to just recommend the change and invite the author for a conversation later if that would be helpful. That said, try to have that conversation over a media that may be shared with the rest of the team, or recollected whenever necessary.

Be respectful

Always keep in mind that there’s a person on the other side of the wire, not a machine, and that it’s hard to understand written words with little context. Avoid letting anger and frustration leak into the review, even if you feel it is justified. There’s no good outcome in those situations.

It doesn’t matter who broke it, or who coded that silly piece of code. If there is broken code, and the project has reviews, multiple people were in the pipeline for that result, and they were trying to get it right. Take shared ownership of the problem, and look for the solution and for how to avoid such issues in the future.

Praise the good work

Reviews carry some low energy feel by their very nature. No matter how positive you are about them, and how much the whole team understands and agrees it is for the best, you are in fact looking for places to put your finger in someone else’s work. For that reason, it is very helpful to take every chance you can of praising logic, design, code organization, or whatever else that you honestly felt was well done. It won’t ever balance it out, but it will at least remind the author that the contributions are welcome.

Suggestions are appreciated

Perhaps a longer variable name would be helpful, or that constant could have a more descriptive name? In many circumstances, the change is indeed subjective, and the gain is pretty marginal. In those cases, if you really can’t resist the urge to say something, a good approach is a suggestion that may be exercised or not at the author’s discretion. Ideally, suggest several options that would feel better to you, so that your point is better understood and agreement is easier. That said, read on.

Avoid trivialities

When reviewing that very simple point, think to yourself: all things considered, does it actually matter? Is the cost of the author’s time, and the potential debate, really worth it? You surely have your opinion about whether to spell “min <= count” or “count >= min“, but so does everybody else. When it’s purely a matter of preference, the author is entitled to have one after all.

Small branches win

Code reviews are useful for a number of secondary reasons, but the primary goal of the code review is to analyze a proposed change, to fix it for inclusion, or to reject it. It’s often tempting to recommend further changes to be bundled onto the same review, but it’s important to keep some focus. Are these additional changes tightly related to the original idea, or would they rather be more appropriate on a future branch?

Also keep an eye on large review submissions. It’s quite rare to see changes of a thousand lines or more that are really an indivisible unit. More often, it ends up like that organically, as a result of the workflow followed by the author. These branches may be very frustrating, both for the author and for reviewers. For the reviewer, it’s hard to keep the necessary level of attention and enthusiasm for the problem over expanded periods of time. For the author, it will be equally problematic to run over a large review. In some extreme cases, it may be worth going back and breaking down the change into more change sets.

Overall, fast iterations on small branches are much more rewarding to work with.

Work with inline comments

This is about tooling, but doing anything else should be considered unethical really. If you don’t have a system that allows the change diff to be seen within the rest of the content, and comments to be made inlined right where you see the issue, implement one right now. Moving to such a system was the most dramatic change in productivity I had as a reviewer in the past several years, and makes the whole experience a lot more bearable for everyone.

Enjoy!

Make sure you’re enjoying what you do, and appreciate what your code reviews are achieving. There’s little point in playing the role of an intelligent computer over extended periods of time if you are unhappy about it. Get yourself your preferred slow-drinking beverage (chimarrão?), perhaps some snacks, a comfortable chair, and relax.