“Peer code reviews are the single biggest thing you can do to improve your code” - Jeff Atwood 1.
We are strong proponents of good code review at Zesty, allowing our remote team to work independently while keeping everybody aware of what the rest of the team is working on and how that work affects the codebase.
We've developed some simple guidelines for our internal team to consider when writing and reviewing code that would be helpful for any engineering team to follow.
Why do we do code review?
There are a lot of reasons to do code review, but the ones that are most important to us are:
Knowledge and skill transfer
When programming alone, it's easy for one person to quickly become the domain expert in one area of the business or codebase. Code review is an organic way to distribute this tribal knowledge and raise our bus factor2. This works equally for existing engineers and on-boarding new ones, and applies from high level business knowledge down to small code skills. Code review is an opportunity to learn something about the business and learn a new programming trick as well.
Code is read far more often than it is written, so it's important to make sure that the person who reads your code six months from now can understand what it does. Oftentimes that person is yourself, so be nice to your future self! Consider the change from an architectural perspective -- is this file in the right place? -- down to a micro level -- is this method named correctly? Does it do what you'd expect?
While it's generally the responsibility of the submitter to ensure that the software is bug free and meets requirements, it always helps to get another set of eyes on a pull request. We like to play devil's advocate and ask 'what if such and such happens?' Knowing that somebody else is going to ask these questions helps you consider edge cases and write better code.
When do we do code review and who participates?
Because we believe that code review is best used as a strategy for distributing business and programming knowledge, it's important that every developer is involved. It's equally important that review flows in every direction, from senior developer to junior and vice versa. Everybody has something to learn from somebody else.
We use a fork of a hubot script3 to randomly assign a reviewer to a given pull request, although you are always welcome to comment even if you are not the assigned reviewer. We encourage all developers to get involved if they have an opinion about something, but the submitter and official reviewer have the final word on when it is ready to be merged.
We like to review and merge pull requests on a regular cadence to prevent pull requests from going stale. Good times to check your assigned code reviews are first thing in the morning, before and after lunch, and right before you leave for the day. Reviewing code is a great way to break up your day and take a step back from a particular issue you are working on.
How do we review code?
Individual styles vary, but we've developed a few simple guidelines that help us achieve our goals.
Small pull requests and granular commits
Nobody wants to review a thousand line pull request. We try to keep pull requests under a couple hundred lines for both our reviewer's sanity and deployment piece of mind. For those pull requests that just can't be easily broken up, the next best thing is to have smaller, granular commits that tell a story and the reviewer can walk through one at a time.
Because our primary goal is knowledge transfer, providing a great pull request description is paramount to good review. Doing so will help the reviewer understand the what and the why of the change.
Another great practice is to go through the file changes on your own pull request and comment and provide extra clarity on anything you think the reviewer might ask about. These comments will help the reviewer understand your thought process, and you may even catch something you didn't while writing it.
Comment on things you like and consider your tone
We like to reinforce good practices just as much as we like to call out bad ones. So if you see something you like, say something!
On the other hand, when providing potentially negative feedback, consider the way you phrase your suggestions, as tone and intent are frequently lost through text. Few things in programming are absolutes, so consider a piece of feedback as the start of a conversation, and ask clarifying questions when in doubt.
To help us with this, we often:
Emoji is a great way to communicate without words and helps leave emotion and misunderstanding out of our comments. Some examples:
🔴 Resolution required. We use this if the code will not be accepted as it is, or until an agreement is reached. We follow that up with a 💚 when it has been resolved.
🐛 Bug! We use this to point out possible bugs. Similarly, 🐌 for performance issues.
😕 Confused. This is used to start a conversation about a piece of code we're unsure about.
🎨 Style. Use this to indicate style or formatting issues, and 📖 for readability concerns.
With a streamlined process like this, code review can be fun while improving the quality of your code and spreading knowledge across your theam team. Thoughts? Suggestions? Feedback? Email me: firstname.lastname@example.org
If you found this interesting, we’re hiring! https://www.zesty.com/jobs