How to make Great Code Reviews™
Code reviews and the pull request are the basic building blocks in many software engineers’ life (The Worflow). They prevent bugs, mistakes, and help distribute the knowledge around a code base in small units of changes. They are, also — and specially code reviews — more an art than a science, and so everyone makes mistakes here. Things as random as the relationship that different contributors have between them, influence whether something is approved, overlooked, ignored or requests changes.
And so for all that it is very important that we try to have a policy than addresses all these different variables, and tries to optimize the user experience (with all that it entails) and developer productivity. I’ll cut to the chase now.
The Policy
- Have a policy. Define before hand what is the threshold to approve or reject a PR and in which cases. For example: Will be rejected if the changes contain a look with a time complexity unnecessarily high; Approved if it’s a typo fix or a one-liner to enable a flag feature; Rejected if it’s a bug fix without a test, etc.
- Stop reviewing for syntax errors. Use a linter. Anything. SwiftLint, Eslint, PyLint, RuboCop…Preferrably something that automates these tasks and do them for you in your IDE or (bonus points) in a pre-commit hook. Syntax errors may prevent the code from compiling in some languages or may be cause an exception when interpreted on your user’s session. Many of the times this can be prevented.
- Reduce or eliminate the talk about style. Code style might be a historic standard (Java), built into the language (Python) or non-existing (Javascript!). But in all of those cases it’s something that is very opinionated and doesn’t add value to the conversation. Adopt a tool (any tool) that lints or automatically formats code so that your team submits everything under the same format and you reduce all the noise. Google-Java-Format, SwiftFormat, Prettier, Really just pick anything.
- Invest in a pipeline. Set up some scripts to run on continuous integration upon merge-request. These are much easier to setup nowadays, many times with options included in the control version provided –as in GitLab, recently supported by Scope.ink 🎉— You can check Circle CI, Travis for most teams. Bitrise & BuddyBuild are interesting products for mobile. If you are still looking for some free optionsyou should check now.sh or glitch.com and automate some routines there.
- Typed languages increase confidence. Some changes are just not going to compile. If you have implemented some git-hooks and/or have CI you can rely on the language itself preventing from a whole range of mistakes. There is no longer the risk of passing the wrong parameters to a function, or the infamous
can't read preperty foo of undefined. This can be extended to other principles: high coverage increases confidence, but does not guarantee quality. - No one likes comments about readability. Few times they are something welcomed by the writer, they probably chose this approach because it was appealing to them in the first place. If a comment is indeed needed it must come with a compelling reason: performance, error prevention or maintainability. Maintainability is something that can become very opinionated very fast. A good rule of thumb is to not question readability if that code path is covered by a test in the source, and specially, the same PR.
- Communicate clearly what’s the information you are missing to approve the changes. This is not to be overlooked. Some times other engineers may come and take a look at the code, leave comments to never return. This is a missed opportunity to add clarity for the next reviewers and the developer themselves.
- Think twice about requesting a “small change”. In a big code source, this may imply having to make further small change in function calls, dependencies, unit tests. Even for tiny bugs and enhancements
Finally, if you are the contributor:
- Have a hard reference in your PR of what you believe to get reviewers approval. Enforce some PR description or have a consistent way to track user stories with software changes. A basic description template can be just asking for a rationale and a test plan.
- Make pull requests as small as possible. They should be as small as the minimum functional change that makes sense. How do you know if you are writing too much, why bothering splitting PRs? Again, this is more an art than science, but you can use Scope to study the amount of changes, files and folders in elaborated diagrams.
Now, how can I get more reviews?
We can’t improve what we can’t measure. It’s important to keep track and visualize what’s going on to move forward in the right direction. Change our habits. And that’s what we do. You can choose however any other tool for this matter, but we bring the topic because it’s important to our vision.
Scope 🔬 can do now so much more, but it was born as a tool to do a just a few things very well:
- Keep ourselves accountable and be able to benchmark our contribution to different projects. See how people make progress through time. How well does our onboarding work for software engineers
- Compensate people for the good work
- Encourage smaller PRs. In Scope the number of PRs gives you more points than their size! That helped us encourage people to split their PRs which makes them way more easy to approve
- Encourage reviews. Sometimes it’s hard to stop what you are doing in your project to contribute to someone else’s. Many organizations (even the top in the industry) lack a way to recognize the work than our peers invest in reviewing. By visualizing the PRs at the same level as reviews, the team in Yeti Smart Home started to give reviews twice as fast.
You can read more about How To Measure Developer Productivity here.
· Jesús Darío · 🐦 Twitter · Author of 🔬 Scope · 🏡 Yeti Smart Home
