Code reviews: the lab meeting for code
While working as a postdoc at CU Boulder I met Rob Knight, a professor in
biochemistry and molecular biology who had a strict discipline of doing, in
addition to his group lab meetings, regular code reviews with all of his lab
members. Rob’s group is extremely active and productive, and I recently asked
him about his continued use of this process. He does still hold them and
explained that he feels they are a key element of his lab’s success; he also
provided some very useful feedback based on which I put these notes together.
With Rob’s permission, I am posting here a set of guidelines that come from his
reply to my questions, which I intend to use in my own work here at UC
Berkeley. I have made minor edits to his notes, but the full credit of these
ideas, fine-tuned and validated over more than 7 years of practice, goes to
Rob, to whom I’m very thankful for allowing me to reuse them here. I do claim
full credit on any blunders I may have added in editing.
These meetings bridge research and practical implementation questions:
sometimes the discussion may focus on fundamental algorithmic ideas or the
approach to a problem, other times the problems will be documentation, testing
or implementation clarity. But I feel that as our research sits on an
increasingly large and complex pile of software tools, many of them written
in-house, we must treat these tools with the attention and expectations of
rigor that we have for other parts of the research work. And the only way to
do that is to scrutinize the code with the same peer-review process that we use
for results within a lab before we put them out in a paper sent for publication
(where they do get further reviewed).
The most difficult part of writing code is always to make it understandable to
other people, including yourself a few months down the track. There’s
certainly no shame in finding out that your code wasn’t as easy to understand
or use as you’d hoped, so don’t take it personally when it happens (which it
always does, at least in my experience), but treat it as an opportunity to
Here’s some general guidelines for making your code review go smoothly:
- Make sure the motivation for your code is clear. Someone who isn’t intimately
involved with your project should understand from the module documentation
and the comments what you are trying to do, what approach you’re taking, and
why they should expect it to work.
- Take some time to prepare a presentation about your code that will answer the
above questions even for someone who hasn’t read the code. You’re more likely
to get useful feedback, rather than nitpicking about syntax, if the audience
can see the big picture.
- Get the code sent out at least a few days beforehand along with some
background about what to look at (if large), whether suggestions should be
about architecture/implementation/algorithm/requirements, etc. Make sure
everyone has enough time to read the code beforehand, and don’t send a series
of updated versions immediately before code review.
- Don’t try to present too much. 200 lines is an absolute maximum – 50 is
usually more reasonable.
- Include examples, either as unit tests or standalone in the module’s
__main__ block. It can often be a lot easier to see an example working
than to figure out the algorithm from the code.
- Ask yourself, before sending the code out, if it could be simpler. Break
complex routines down into smaller ones, consider using built-in functions
and data types instead of custom ones, see if you can separate complex logic
into a dispatch routine and simple individual functions, etc. This also
generally helps you make the code more testable.
- Follow the coding guidelines and Python idioms applicable to your group. At
UC Berkeley I am following for all of my projects the Python style guide
encoded in PEP 8, supplemented where necessary (in particular with regards
to docstring standards) with the Numpy coding style guidelines.
- Take advantage of existing mature code in your group, the Python cookbook,
and other sources of existing code to get an idea of how to do things. Many
problems have already been solved for you. It’s always better to reuse
well-tested code (if it does what you want) than to reinvent the wheel.
- It’s OK to present code that doesn’t work yet, or that you think needs
improvement, as long as you signal this fact very clearly in your email
sending out the code. If there are specific areas you want people to focus
on, don’t keep it a secret – it’s a waste of time to give detailed feedback
on code that is about to be scrapped.
Suggestions for the meeting leader
Rob also provided the following tips for the person leading the meetings, which
I think are very valid and complement the general guidelines above for the
entire group. These are mostly about setting the right tone for the
discussion, so that a productive culture develops where the process can be both
rigorous and friendly.
- Keep it a safe environment, i.e. make sure chastising is relatively gentle
even when deserved (but do point out when code doesn’t meet the required
standard – frame it as a learning experience though).
- Make sure there’s a core of vocal participants so it isn’t always you.
- Make it clear when you’re presenting yourself that your code isn’t perfect,
point out some of those imperfections yourself if audience is slow to do so,
do present yourself.
- Patiently explain when things are not wrong but just stylistic differences
(but make it clear that some styles are bad, often helpful to e.g. ask
people to guess what a function returns from its name).