Writing Code for Better Reviews
- 1. Make your intentions understood
- 2. Group changes by intention, one intention at a time
- 3. Break up large changes into multiple reviews
- 4. Avoid high-noise, low-value changes
- 5. Establish shared hygiene to reduce noise
- 6. Rebase with care
- 7. Review your work before asking others
- 8. Understand your review tool
I believe code reviews are a high-value activity (which I’ve written about before), but they take time and slow down your development process. With a few simple tricks, you can make it easier for reviewers to understand your changes, allowing them to give you better feedback faster. Not only does this save everyone time, but it also improves the quality of your code.
Make your intentions understood
Good commit messages and review titles are important. It may be (and in fact, should be) obvious from your code change what you’re trying to do, but a good message is still important. There are lots of cases where someone needs to scan the change list, and good messages make this a lot easier. It also helps your reviewer understand what you’re doing.
The key to a good commit message is describing your intention. “Added some code” may be correct, but it’s not helpful. “Check for invalid email addresses” explains why you added the code. Your audience is a future developer trying to figure out why a feature broke, and what they care about is getting a quick picture of what you could have changed.
A little humour can be fun sometimes, but be careful with this. Small tasteful jokes are fine, but sarcasm or jokes that obscure your meaning will just slow others down.
Group changes by intention, one intention at a time
I tend to meander through changes as opposed to attacking them linearly, and I think this is pretty common. I run into small things while I’m wandering through the code, and I want to fix them right away. I could take a note and come back later, but this sometimes feels like moving backwards. The compromise that I normally go with is fixing the side-thing anyway, but presenting it separately.
If the change is very small (ex: a typo in a private method name - the fix will only affect a couple lines of code in one file) it may be okay to tuck it in. If it’s only a few changes but spread across a few files, putting it in its own commit is better.
The worst offender is the sweeping change. No matter how much a typo in a widely used function name might bother, adding a couple of hundred lines of noise to the review is absolutely going to slow it down. A new, separate review is a great way to present these. It’s a lot easier to verify simple changes if they are the only kind of change present. With Git and GitHub, it’s pretty easy to rebase a single sweeping change commit into its own branch and put it up for for review immediately.
Some “simple” fixes are not as simple as they seem, and could introduce a bug. If there is any doubt at all, it may be worth separating these changes so that they get separate scrutiny in the test and documentation pipelines. This usually means creating a new issue in your issue tracker. If they’re small enough, and if your tools allow it, you might still be able to include the change (in its own commit) in your same code review.
Break up large changes into multiple reviews
Sometimes a single change is big enough that you may want to break it into smaller pieces. For most cases, multiple commits may be enough, but for the largest changes you may want multiple reviews. This can be especially kind for a reviewer, or helpful if a change spans multiple areas of expertise.
One common and sensible rule is that every review should be able to build and pass tests before it is merged. That needs to be the first consideration when breaking a change down into smaller reviews. It can be temping to break up reviews by folder, or application layer, or some other simple delineation, but it will make it harder for reviewers to understand what is happening.
Avoid high-noise, low-value changes
Reordering code, rearranging imports, or running automatic reformatting functions can create a lot of changed lines in a review tool. Worse, it can sometimes be mistaken for removal and re-addition of code, making it harder to notice any changes made at the same time. This also makes it harder to track the true history for your lines of code. Sometimes there is enough value to justify the noise, but make the decision consciously.
Establish shared hygiene to reduce noise
Using an explicit .editorconfig
is an easy starting point. Automatic reformatting tools can save a lot of time. The noise should be minimal if everyone follows the same rules.
There are some other practices that can help reduce noise:
- use trailing commas in lists (if your language supports it) - this prevents the last line from changing when a new line is added
- keep large lists in alphabetical order - this makes it easier to find code, and prevents the temptation to regroup and reorder things
- keep code files small (ex: one class per file)
- keep auto-generated code in separate files from custom code (ex: partial classes)
Rebase with care
Avoid rebasing when a review is underway. Sometimes a rebase is necessary, but it can disrupt the change history that reviewers are in the middle of processing. GitHub can sometimes be pretty bad with this. It’s okay to rebase before anyone has looked at your review. If you have no choice, see if you can delay the rebasing until all feedback has been addressed and accepted.
Review your work before asking others
Quickly scan your changes before sharing them with others. I have caught innumerable typos or accidental changes this way. You could leave them in the review, but it wastes time for your reviewers, and it makes you look sloppy. It can also add an unnecessary iteration to the review process if there was no other feedback.
Understand your review tool
There can be subtle differences in the reviewing experience depending on the tools and process your team is using. Take time to understand how your tool works, and make sure you follow the processes your peers prefer.