Junior software engineers receive many comments on their pull requests—often about how their code looks and other minor issues like having unused imports—and have to do additional fixes before the team does a “real” code review. Developers who are more experienced, newly assigned to unfamiliar teams and projects, have trouble making the reviewers focus on what the code does rather than remark that they’re unhappy with the usage of a less-popular functional operator.
How do you anticipate the (unexpected) questions about minor details you haven’t thought about that seem to be more important than all the other details that you specifically have, and have mentioned them in the PR description? How do you shift the conversation to the structure and the construction of your code, and how you can improve? The truth is, there are no “true” programmers who push perfect code every time, and it doesn’t really matter how many fixes you make during code review. I’ve seen senior engineers spend weeks working on their PRs, addressing feedback—it doesn’t make you a lesser programmer.
Implicit knowledge
The difference between someone who has been on a team for a while and someone new, or a junior software engineer (who is also new to the project—a double whammy), is that they have acquired and are using implicit, unarticulated knowledge about a project that the new person lacks, informing their decisions when writing new code and reviewing the other team members’ work.
Even a team lead can review your code guided by the feeling whether she “likes” it or not, and if it “looks good” to her. While this attitude has merit (see, for example, Joel Spolsky’s post “Making Wrong Code Look Wrong”), it is detrimental to us, the newcomers, who do not yet have this “radar” because we haven’t spent enough time with the team and the project.
What we can do—at any level, even if we’re junior engineers—is to bring the team members’ feelings to the surface and make the heuristics they use explicit (and public).
The PR checklist
Our intention is to create a checklist that we will go through when submitting a pull request that will help us eliminate most of the guesswork of ranking the PR as “good” or not, and most importantly, to promote constructive comments—because we’ve taken care of the minor details.
The PR checklist is a shared document with shared agreement from all team members, so it will make not only your, but everyone’s life easier. It’s also not large or hard to maintain. Here’s what you can include:
- Small details that can be fixed before the PR is reviewed. Use comments from your own PRs as a starter :) These can be unused imports, typos in doc strings or variable names, misaligned lines and all the other things that elicit comments because they are easy to spot.
- Pull request description formatting and template. In my experience, having a short explanation of what the code change is supposed to achieve, along with links to a spec or related tickets, goes a very long way towards the quality of code review. It is the difference between someone thinking “Uh, I have to write something about this piece of code, what does it even do?” and “OK, this is supposed to do X. I noticed a logical mistake here when comparing against the spec, let me write a comment…”
- Points relevant to your team and project. This is the hard part that you will have to extract from your teammates. You can get more specific information by asking to describe, in detail, what they have thought when reviewing previous code changes, and what qualities they are looking for when reading code.
On the whole, sounds simple enough? If you want some inspiration, here is a checklist for creating a pull request from one of my previous companies, Babylon Health. The other sections of the document are also worth checking out.
Now, when submitting a new pull request, you’ll be (reasonably) sure that it ticks all the boxes, and the comments that you get will be about the essence of your code and not its presentation.
Promoting shared values and shared ownership of code like this naturally expands into coding guidelines and generally a more open engineering culture, which is conducive to learning and self-improvement, and results in a kinder—and more productive—team environment.