Teams use code review in different ways, and your experience may vary depending on who is doing the code review, but there are always things you can do as an author to make the process go more smoothly.
When submitting changes for review, you should try to reduce the cognitive load on the reviewer. As an author you have spent way more time thinking about the problem than the reviewer will, so what is easy to read for you isn't necessarily easy for them.
Why it matters
It's always tempting to take shortcuts and throw something up for review quickly, but I think it's always worth pausing first and considering what we are demanding of our teammates. Bear in mind people don't operate at 100% all the time. Your reviewer may be having a bad day, they may be tired or sick, they may be context switching between different tasks. Make their lives easier.
Complex pull requests are also slower to resolve. At best, you have to wait ages for feedback (which is frustrating if you want to move onto new tasks), at worst, the reviewer focuses on surface level details, rather than giving you feedback that is actually useful (e.g. about the correctness of the solution, or maintainability).
If your team follows Github Flow, or similar workflows where reviews block merging, then the longer reviews take, the less frequently work is integrated, the more unfinished work is hanging around. In my experience this leads to extra work in the form of conflicts, deployment issues, and coordination between engineers. By doing a bit of extra work up front, you can avoid extra work later on.
Tip 1: Start with small commits
Development can be messy, and the code we start with is not necessarily the code we end up with when we are ready to push our changes. But it's easier to combine lots of smaller commits later than it is to break apart big ones, so commit early and often.
Capture your reasoning as you go. You can include
- any assumptions you're making
- anything you've learned about the code since the previous commit
- things you've thought about but not changed
This is stuff you will forget if you don't write it down.
You can rewrite these messages before you push the code, so it's better to overexplain than leave something out and then forget about it.
Passing a -p (or --patch) flag to git add is really helpful. It breaks your changes into "hunks" and you type y or n after each one to choose what gets staged. That way you can create more than one commit even if you've changed multiple things since your last commit.
Tip 2: Learn how to rewrite your git history
When you're committing as you go, you might end up with a
simple sequence of commits where each commit builds on the previous one. This is great, because the reviewer can look over the work commit-by-commit, focusing on one thing at a time. Essentially, your commits tell a story.
If you're working on something complicated, your commits will probably be messier than this, but that's fine if you rewrite the git history before sharing. This way, reviewers don't need to relive the whole journey you took to arrive at the solution.
I regularly use some combination of:
- rebase to remove conflicts with the main branch
- interactive rebase to squash many commits into one or edit commit messages
- cherry-pick to pick out commits I care about into a fresh branch which can be reviewed separately
- reset to completely redo some commits while keeping the code I have in my working tree
These commands can be pretty confusing, but they are worth learning, and you can undo almost anything in git if it goes wrong.
Tip 3: Squash commits that don't provide new information
Similar kinds of changes can be squashed together before sharing for review (see above). You might do the work in stages - so you have lots of checkpoints to rollback to - but then present it as one logical step in the PR. This makes it much easier for the reviewer to check that the change was done consistently across the codebase.
Prefer commits that are small in scope but "complete". If you are aiming to write tested, documented code, then the perfect commit might contain implementation, tests, and documentation.
Don't push code where you try something in one commit and then undo it in another. This is confusing: it adds noise to the commit history that makes it harder to review the individual commits in your PR. Each commit should make sense on its own.
I deal with these problems using an interactive rebase:
git rebase -i <commit you branched from>
This asks you what to do with each commit on branch. squash combines it with the previous commit, drop removes it entirely, edit lets you amend the commit (which you can use to change the commit message).
Imagine you were doing the work from scratch - which steps would you take? If you make the existing commits match those steps then somebody else should be able to follow along.
Tip 4: Stop when you have something useful and deployable
The more changes you have on a branch, the riskier it is to deploy and the longer it takes to review.
If your branch gets out of control, look for any commit part way through that is deployable. Make that your pull request, get it reviewed and deployed, and then come back to the following commits once that it is merged.
git checkout -b <new-branch>
git reset --hard <deployable-commit>
This can feel slower, because you're waiting for things to happen before doing the next part, but it's actually faster. You can get code into production earlier, reviews happen quicker, and if something goes wrong you can fix it quicker.
You may find that if some refactoring was done first, then the rest of the pull request would be simpler to understand. In that case, you can raise one PR for the refactoring, and then redo the original PR. That way, the first review only needs to consider whether the code functions the same as before, and the second review only needs consider the impact of the new change.
Tip 5: Review and edit your commit messages
The commit message should help reviewers review your code quickly.
Reviewers need to
- understand what has changed
- understand the bigger picture of what your change enables
- follow your thought process
So provide this information in an easy to digest form.
When code is presented without context, the reader has to rediscover the context themselves and make assumptions about what you were trying to do (which could be wrong and waste time).
If you reference an older commit, include the commit hash. Then anyone can find the commit using the CLI, and it will be hyperlinked in github.
If the work was prompted by a ticket, link to the ticket. But it's worth also including a summary of what you are trying to achieve, so that people looking at the repo in the future can still understand your code, even if the ticketing software is no longer used.
If you fixed a bug, link to any external information you used to understand the problem, e.g. manual pages or CVEs.
Tip 6: Keep pull request descriptions short and to the point
If the commit messages are detailed, then your pull request description can be a short summary or introduction, to give the reviewer a basic understanding of what you set out to do and why before they dive into the details.
Other things you can include in a PR description:
- A discussion of the problem you are solving and how chose to approach it
- Specific questions for the reviewer, or areas you want to highlight
- A discussion of trade-offs you made
- Screenshots for any UI changes
The pull request is for the team right now, whereas the commit messages are part of a permanent record that your future self/team can refer back to.
Tip 7: Refactor code to eliminate confusion
If something is unclear to the reviewer, use the opportunity to make it clearer, for example by:
- improving how you name things
- splitting or inlining code
- improving commit messages
- adding comments to the code
Tip 8: Don't bite off more than you can chew
If you're starting something complicated, consider treating it as a spike and then redo it from scratch once you know how you want to implement it. I like this approach because the whole team understands that the work is experimental and can be abandoned if it's not going to be worth the effort to continue.
It's ok to throw everything away and start again, building on what you've learned about the problem so far.
Top comments (1)