Our way of doing code review and pull request
At homes.com.au, our engineering team has developed our own approach to conduct code reviews and pull requests (PRs) that enables us to move fast and not break things (as much as we could, that is).
TL;DR
We embrace a hybrid approach of trunk-based development and PR model. There is no hard and fast rules. We will use whatever makes the most sense in a given circumstance. We acknowledge that it is okay to not use PRs when appropriate.
However, we encourage people to use a PR, and review code, if:
- There is something in the PR that you want to share with other engineers
- You want to learn from each other’s code
- You want to discuss the solution and ideas with the team, perhaps even coming up with a better alternative
Background
To give it a bit more context, prior to settling on an agreed approach, we enforced PRs for all changes to our codebase, via the repo settings on BitBucket. It worked fine — but not great — as common problems started to emerge; such as PRs were getting blocked from merging due to slow review, pushing hot fixes for production issues took longer as they needed to be approved and merged.
When we migrated to GitHub (one of the best decision we ever made!), we took the opportunity to rethink our process — effectively how we could make it more enjoyable and efficient. There were benefits to the PR model that we didn’t want to lose. For instance, the ability to have another pair of eyes to review the code to ensure code quality, catch unnoticed bugs or missed requirements.
At the same time, alternative approaches such as trunk-based development was lucrative as it would enable us to move fast — given we were an early stage startup. As reviewing PR can be time consuming, it can be frustrating for the PR author to wait for approval as well as resolving discussions — due to the asynchronous nature of it. Trunk-based development did sound attractive, but we felt that we were not ready for it yet. It comes with its own problems and requires a certain level of discipline from the team to adopt it well.
In the end, we agreed on a hybrid approach and trusted our people to pick the most appropriate choice. It also aligned with a principle in our engineering manifesto:
“We give our people autonomy to make the right decisions.”
Guidelines to write a good pull request
No two pull requests are equal. There are ways to improve them, such as adding more context, that makes reviewing them a more satisfactory experience. Here are some guidelines we look out for to make a better PR:
- Meaningful title and description
- The smaller the change the better. Would be even better if it is atomic
- Well-maintained commit history
- Add visuals such as images, videos or diagrams whenever possible
Most importantly, at the minimum engineers should explain why they are making this change.
Suggested format
Following the broad guidelines mentioned above, we came up with a suggested structure to the PR with the aim that it would help articulate the changes. Having said that, these are still suggestions and not every point must be included every time.
The why
- Why is this change needed in the first place?
- What problem does it solve?
- Any background or context the reviewers need to know?
The what
- What was changed?
The how
- Reasons for choosing this approach
- Have we considered any other approach?
The bonus
- What else do reviewers need to know?
- Any gotchas?
- Is something still WIP?
- Links to a reference materials — such as articles or (internal) documentation?
Examples
An example speaks a thousand words so we have 2 examples of 2 PR descriptions that we will try to apply the principles mentioned previously to gradually improve them.
Example #1: a simple UI change
👎
Update margin of the map component
👍
Update margin of the map component so it is in the centre of the page
👍👍
Update margin of the map component so it is in the centre of the page based on peer review. Confirmed the design with the Design team.
Example #2: a more complicated change
👎👎
Set lambda timeout to 10 seconds
👎
Increase lambda timeout from 5 to 10 seconds
👍
Increase lambda timeout from 5 to 10 seconds.
The lambda was timing out intermittently when processing large files.
👍👍
Increase lambda timeout from 5 to 10 seconds.
The lambda was timing out intermittently when processing large files.
I’ve tried to increase the memory for the lambda but it didn’t help since we’re already processing the files by chunks. The file server is not controlled by us so there’s not much we can do on the other side of processing. Increasing timeout is the only option.
We will probably need to implement a retry mechanism when errors happen. But it seems to be quite complicated; I would need to learn how to implement it first. Will do that in a separate PR.
Guidelines to code review
Respect other people’s time — from both sides of a PR:
- Reviewing a PR can be time consuming
- Waiting for a PR to be reviewed can block other tasks
Always provide constructive feedback:
👎
This is just wrong. Why don’t you use X to implement this?
👍
I suggest we use X to implement this since it will be more concise and actually is an idiomatic pattern in Y language. See the docs here [provide link].
Keep the ego out of code reviews:
- Seniority doesn’t always mean being right
- Provide reasons, not feelings
Feel empowered to defend your solution, with clear reasonings and probably links to articles or docs to support your point.
When in doubt, ask. There’s no stupid question.
Written arguments can be easily misunderstood, resort to a quick call or in-person chat if arguments get heated, comments/replies drag on, or there are too many “I don’t get it” type comments. Post a follow-up comment summarising the discussion.
If suggesting an alternative approach, be specific what that should be. Even better if concrete examples are given.
If there are shortcuts or temporary solutions made, add explicit comments and/or TODOs so other people know about them and there are records of them in the code. No record means it will never be addressed — like a tech debt.
Review the PR keeping the bigger picture in mind. In a complicated system, applications (or services) are often coupled. For example:
- Changing a field type in an object model can affect other applications using that model. Make sure you’ve checked all other repositories that may use that model.
- If it’s a breaking change, consider how the system will behave during deployment. Will there be any downtime. If there’s a failure, how do we rollback. We must consider all possible scenarios. Reliability and availability don’t come for free easily.
You don’t need to be a senior engineer to review a senior engineer’s code or a frontend dev to review frontend code. Everyone is free to review other people’s code, even if the purpose is learning.
Conclusion
It has been 3 months since we started applying these practices and the feedback from the team members have been positive. Straightforward and non-code-related changes can be committed directly to the main branch. It results in less wait time for such trivial changes to be merged and enforces another principle in our engineering manifesto:
“We take full ownership of the things we build.”
That means more happy engineers 😃. Otherwise, the team still actively use PRs and reap the benefits of high-quality code reviews.
In conclusion, we are satisfied with the choices we’ve made — adopting best practices from the industry and adjusting them to fit our team dynamics. We understand this is not the end and that, as we grow, we may need to revise the process further.
Thanks to Benny Thomas for reviewing this article.
We are also actively looking for awesome engineers to join our engineering team. If you share the same view on these values and are interested in solving exciting challenges with us, please get in touch https://www.linkedin.com/feed/update/urn:li:activity:6861441278274629632/.
