Code review is a common practice for professional software teams. As an author, there are several things you could hope to gain from having your code reviewed:
- Other people’s opinions on stylistic and naming changes
- A rigorous defense against logical and syntactical flaws
- A chance to communicate better and share knowledge with your peers
- The respect and adulation of the aforementioned peers
In reality, nobody genuinely wants the first one. Automated testing is far more effective for the second. I will only focus on the latter two gains. The real ones.
Keep the change succinct
Keep your pull requests both small and focused. If you move stylistic and refactoring changes into separate changesets, it will allow reviewers to focus their mental energy on understanding and internalizing the important logical changes in isolation. I am a fan of conventional commits, which lends itself to this naturally. Calculated and focused commits also reduce the chance of you creating a wall of code that nobody ever finishes reading.
A small PR is a reviewed PR.
If you find the description you are writing contains several related but not tightly coupled changes, consider turning the pull request into a draft, and isolate the changes into their own reviews. This reduces the cognitive burden of the reader, allowing them to focus better. It also gives you a chance to give your code a quick pass first.
Add context
You spent all this time being great, make sure everyone else understands just how great you are. A poorly capitalized sentence fragment as a title for a pull request shows your reviewers how little you value their time.
Whereas, a detailed description, with links or references to the associated ticket gently indicates that maybe this is worth more of their time and scrutiny. The time the reviewer saved trying to understand what it was they were supposed to be reading can now be spent returning value back to you.
feat: Add support for the legendary MultiFrobulator
As part of the effort to increase maximal Frobulation, this
adds support for the experimental, yet promising
MultiFrobulator. There is a subtle concurrency
gotcha that should be mitigated with the extra Glurm.
Do not operate after a large meal.
ticket: FRB-2021
Let your first reviewer be you
You can still have code review on a team of one. Once you have pushed your code and kicked off your automated tests, take a walk, nap, or ride a bicycle to a karaoke party. Once you give your brain a chance to recharge, try reading your own PR. Be liberal with minor suggestions. Anything that was not immediately obvious to you will likely be far more confusing to other readers, or you again six months later.
This also makes a good time for documenting your thought process. You know, the transient comments that matter the most in the first 24 hours of a change and are absolutely worthless 197 commits later. By preemptively commenting on sections, you can encourage dialog with your other reviewers. It also highlights important areas they may not have known required more scrutiny.
Stop with the rebasing already
Once you finish your initial personal review, make a promise to the universe that you will not rebase for the lifespan of the pull request. While rebasing (and amending commits) can result in a clean, expert looking commit, it robs your readers of the learning opportunity to see your thought process. It also has two very negative effects on your reviewers.
Assume your reviewer will be pulling your code locally and conducting at least a
partial review in their IDE. Both IDEA and VSCode have dedicated pull request
review functionality. With most modern languages, to genuinely understand what
is going on with type inference, polymorphism, implicits, and even just library
calls, the IDE is a necessary tool even for reading. When you rebase or amend,
you are generally also force pushing and making it incompatible with the branch
your reviewer was already interacting with. This causes your reviewer more
effort for your own vanity. This will result in a disinterested disengaged reviewer.
The second negative is that any feedback already solicited gets washed away. Either the comments do not line up correctly, or the changes you did implement become hidden. If you only updated 3 out of 4 similar suggestions, you are forcing your reviewer to start over from the beginning. This will result in a discouraged reviewer.
Never rebase once that first reviewer comments until you are ready to merge. Squashed merges accomplish the same streamlined final commits without introducing the problems generated by rebasing.
All that glitters is not code
One aspect of code reviews that seasoned developers rarely enjoy is unwanted style suggestions. Unfortunately, when people have less experience reviewing code, the stylistic and pedantic commentary is simplest way to start. We emulate what we have seen, and your very first reviewer was probably a compiler, or a linter.
A healthy team will have a general guideline and working agreement for what is considered appropriate suggestions for reviews. In addition, it is helpful to have a common shorthand to convey the difference between required changes and light suggestions. Conventional comments exist for this purpose, similar to the conventional commit messages.
Even with guidelines, gold plating suggestions are inevitable. While it can be frustrating, try to assume it is not coming from an antagonistic, or inexperienced place. Consider, perhaps, that the pull request could have started with a more detailed description, better definition, or more breadcrumb commentary to guide the reviewer. Even if they do not realize it, the gold plater may just not have enough context of the problem or understanding to ask deeper and insightful questions. It’s not that they don’t care, but you may not have given them a reason to.
In Review
If you take a small bit of extra time before throwing your code out for review, you can solicit much more valuable feedback. This serves far more value than reviews solely for the purpose of checking a box.