Andy Herd, 26th May 2020
Recently I asked some friends and colleagues of mine for their thoughts on code review. They shared a wealth of useful comments, many of which were particularly enjoyable because of the fact that they were so strongly opinionated!
The exercise led to some really interesting discussions. Maybe these quotes could help you start similar conversations at your organisation?
What is code review and why is it important?
An opportunity for collaboration and learning
“I learned how to Python well mainly through review comments”
“Code review is a sharing exercise, not just a ‘this is correct’ exercise.”
“I learn just as much from reviewing other people’s PRs as I do from constructive comments received on my own PRs”
Code review enables visibility of what’s changing
“There’s so much going on in our team. Looking at my colleagues’ PRs and code reviews is sometimes the only way I can keep up with all the changes that are being made in our project.”
We release often, so a bug can affect everyone
“If a bug gets into the master branch, it can hold everyone else on the team back until it’s fixed. This is why we’re biased toward taking time to both go through code review and test changes before release.”
An opportunity for asking questions
“The most important thing I have learnt for reviewing code is to phrase as much as possible in the form of a question.”
“There is every chance the person writing the code has thought of the same thing you are suggesting and ruled it out for some reason that you aren’t aware of.”
For both praise and constructive criticism
“I normally try to find something positive to say when I’m writing a code review”
“Just the other day I received both positive and negative comments on a PR of mine. I was really glad that the reviewer noticed and commented on the approach I took because I am really proud of this.”
Expectations of a reviewer
Are you the right person?
“Make sure you are qualified to give a review. Are you a domain expert? If not, within the context of the change, can you give a comprehensive and safe review?”
Any level of seniority
“I think it’s vital to have people reviewing code of people more senior than them, and a culture where they feel able to ask questions and pick holes if necessary. This emphasises that the goal is knowledge sharing and improving code quality.”
“I get a huge amount of value by my code being reviewed by colleagues who are less senior than me.”
When to request changes (and how to do so tactfully)
Use your judgement
“Don’t be stubborn about things that don’t matter, but do ask questions about things that are confusing. Someday it might be you who has to figure out what this thing does.”
“Don’t ‘require changes’ unless you really require changes. It’s [not useful] to do this on a code review where you are just expressing esoteric personal preferences.”
“If you can let go of the things that don’t matter so much to you, you can build currency with others and earn their trust when you do wind up pushing back.”
– Cap Watkins https://capwatkins.com/blog/the-sliding-scale
In person discussion can add more context
“The most efficient and effective method of conveying information to and within a development team is face-to-face conversation.”
http://agilemanifesto.org/principles.html
… as long as those comments are delivered tactfully
“If you’ve gotten into a contentious debate over something in your code review don’t suddenly show up looming behind the other person’s chair. It makes me anxious when my colleagues do this.”
“Talking in person is great but message the other person first so that it doesn’t feel like an intimidation tactic.”
A record of code review is often helpful in the future
“If you talk in person, put a summary on the code review so that other people aren’t left out.”
“The reasoning behind a particular piece of code is often more useful than reading the actual code itself”
“Dead” code
“If I’m trying to understand how code works, sometimes I can spend lots of time trying to understand the existing code to try to determine how my changes should fit in, only to later find that parts of the codebase can never be reached. This is frustrating because it doesn’t feel like an effective use of my time.”
Broken code
“In my experience, broken code doesn’t work.”
“Testing is our main way of spotting bugs. However if in a code review you see something that you think will cause a bug if released, you should absolutely request changes on that PR.”
Comments which detract
“Comments aren’t code meant for the compiler, they’re words meant to communicate ideas to other human beings.”
– Jeff Atwood https://blog.codinghorror.com/coding-without-comments/
“If the code is littered with comments which don’t add anything to the code, or which say the same thing as the code, then it makes understanding the code significantly more difficult.”
Dos and don’ts of code review
DO be mindful of the language you use
“Always use language which enforces shared ownership
e.g. we, ours instead of I, you they”
DO ask questions instead of making statements
“I always try to frame suggestions as questions rather than commands
i.e. ‘Can this be null? If so what happens?’
rather than ‘This will crash here if this is null’”
“Your job as the reviewer is not to be some upper form of knowledge dictating how things should be done, you are sharing knowledge as peers and giving things another pair of eyes.”
DO make it clear how important your comments are
“The change I’ve suggested is not necessary to change in order to release this PR, it’s just a hint for future writing clearer/more concise code.”
– comment received on a PR of mine a few days ago
DO make changes in the same PR as the comments
“If there is a new PR that needs to be created, remember to move comments from the previous PR or tagged so that previous issues addressed are visible.”
DO keep your PRs to a reasonable size
“Small regular reviews of iterative merges are less fraught. As a reviewer, being thrown a thousand line change representing weeks of work is frustrating if I realise there is a fundamental flaw to the entire approach.”
“Deliver working software frequently […], with a preference to the shorter timescale.”
http://agilemanifesto.org/principles.html
DO fill in detailed comments in the PR description
“Why is this change being made?”
“How has the change been tested?
“What prerequisites need to happen before I can release?”
“Are there any steps which must be performed after release?”
“I really like it when I find a PR of mine from a couple years ago in which I wrote a lengthy and detailed description, because I can’t remember most of it.”
DO take ownership of your PR
“The original author of the PR should be the one to merge and release once approved. It is assumed that the person who developed the code would know how to release it, and has done adequate testing to check for any issues after release.”
“After it’s released, does it actually run?”
DON’T hit the ‘request changes’ button if your only comments are only about code style
“We all do things in a slightly different way. That’s fine. The reason we have black and isort is to take a lot of this burden away.”
DON’T make your comments personal, or bring emotion into code reviews
“Assume positive intent.”
– FanDuel values
“It’s not personal, Sonny, it’s strictly business.”
– Don Corleone
DON’T merge “dead” or untested code
“Examples of this are yaml files that are never read, functions that are never invoked, classes that are never instantiated. If we don’t need it, we don’t want to have to maintain it.”