A Patterns Story: Code Review
It's more than just how you do pull requests
An ineffective feedback process can have many causes. And while the problems manifest in certain steps, like a Pull Request, the causes and solutions are often elsewhere.
To get a sense of how the Code Review Patterns work together to help the team get feedback consider this scenario. This specific scenario assumes that all changes are reviewed before merge, but one can imagine other scenarios, for example, only reviewing code with a certain risk level.
After a developer has completed a Small Development Task in a Task Branch they’d like to make sure that they get some feedback on their code before they merge to the Main Line. Since the Small Development Task is small, the change set will be small and quick to review.
While there are other opportunities for feedback during development, a last look before merging will be valuable. There are a few options for getting this pre-merge feedback. This team uses a Pull Request , because:
The platform enables some useful build automation,
It facilitates communication when the team is distributed,
It allows developers who are not feature partners to give feedback if they have the context and can do so promptly.
It allows others visibility into the flow of changes so that they can learn from them.
Because there is a Team Focus on delivering a certain feature set, the team has agreed that the feedback is a priority, so team members can give feedback promptly.
In the past, the team had been working in silos; the stated goal was to increase efficiency, but the team realized that it didn’t really improve delivery, so the team has agreed that everyone will have a Feature Partner who will be a design partner for a given task. (Some developers and their Feature Partners even work using Pair Programming). Because there is someone who has been involved in the design, and understands the goal of the feature, the feedback they give is useful, focusing on design and big picture items.
Issues like style and formatting, and coding approaches that have known vulnerabilities and problems, are identified using Automated Checks, freeing the humans on the team to focus on larger-scale issues, and saving them time, which facilitates quicker review.
While the Pull Request mechanism allows for asynchronous feedback, it doesn’t mean that all feedback is asynchronous. From time-to-time the team will review code synchronously, perhaps after having some time to collect their thoughts. Feedback from the pull request is meant to improve the code, and the Pull Request won’t gate a merge, unless a developer finds a significant issue, in which case they will engage with the developer.
Once the developer has digested the comments, they make any changes and merge the code into the Main Line.
All of these steps allow for the feedback process to be timely, relevant, and actionable. With this ecosystem in place the team can usually merge changes within a few hours after the code is completed. (And sometimes sooner depending on the timing and the team’s working agreements.)
The above was an example of how a patterns-based approach to looking at the code review process might be useful. If you don’t want to think about patterns, think in terms of structures or process steps that depend on each other. It takes more to get good Code Review feedback than a checklist or an assertion. The work, and the rest of the process has to support it too.
A Note on Pull Requests
Two main pain points about branch-based work flows are:
Long Lived Branches
Pull Requests
A Pull Request is defined in the Github Docs :
Pull requests let you tell others about changes you've pushed to a branch in a repository on GitHub. Once a pull request is opened, you can discuss and review the potential changes with collaborators and add follow-up commits before your changes are merged into the base branch.
A Pull Request, then, is:
A way to announce that code is “ready to merge into the Main Line.”
A way to request feedback (synchronously or asynchronously).
In common practice a Pull Request is asynchronous, but that need not be as I’ll discuss in the Pull Request Pattern.
Many of the troublesome artifacts of a Pull Request Process have roots in larger team environment. These can be avoided, though it takes discipline. For example, a Pull Request doesn’t require that reviews be slow, or gate my mandatory reviews. While a slower, gated process makes sense on an open source project, it’s counter productive for a product or project team.