Improving the code review process is crucial for development teams aiming to maintain and elevate their efficiency and code quality. A growing list of pending pull requests (PRs) can be overwhelming and even demoralizing. By refining the review process, teams can ensure a balanced distribution of PRs, preventing certain team members from becoming inundated while others remain idle. Striving for equality in time spent on PRs helps in fostering a cohesive and harmonious team environment. Furthermore, adhering to a standard that aims for a review time of up to just one business day guarantees swift feedback and fosters a dynamic, responsive development cycle. Lastly, the essence of the improvement drive is to enhance the quality of PRs, ensuring they are optimally sized, adequately prepared, and comprehensive, which in turn streamlines the entire development process.
How to measure Code Review effectiveness
To gauge the efficacy of code review processes, it's essential to focus on team performance rather than individual contributions. Key metrics include the duration a pull request (PR) remains under review and the time a PR is in the review phase relative to its total lines of code. Monitoring the number of PRs that remain open and the volume of code within each PR can further provide insights into the efficiency of the review process.
- Measure the effectiveness of the team rather than effectiveness of certain people
- Measure time for how long PR stays in review state.
- Measure time for how long a PR stays in review state divided by lines of code.
- Number of PRs in open state.
- Lines of code in PR
This can be achieved by introducing tools for collecting information about PRs. If you use Github there is a github action issue-metrics that can measure the average time of PRs staying in review for a certain period of time. But you can use Github API to create your own action that will collect information important for your project.
Prepare your PR
Always start by adding a detailed description to your PR. If a visual representation would help, consider including diagrams. Proposing a review plan can also be a game-changer, guiding reviewers through a logical sequence, ensuring no change gets overlooked.
Before you request others to dive into your code, look through your draft PR yourself. This self-review allows you to catch and clean up any stray comments or oversights.
Comments within your PR should clarify your coding decisions, especially if there's potential for confusion. For instance, if you addressed an unrelated bug during the current PR, mention it. This preemptive clarification can save reviewers a lot of head-scratching. Annotations act as a roadmap through your changes. By directing reviewers to specific files or explaining the rationale behind certain modifications, you make their job simpler. The added perk? You might spot errors during this annotation phase, fixing them even before the review starts.
If you've addressed multiple issues in a PR, it might be the case that you need to divide it. PRs should be concise and focused. A rule of thumb: if a change touches more than five files, took over a day to draft, or would require an extensive review time, split it. For example, one PR could lay out the API for a new feature, and a subsequent PR could present the implementations.
How to review PRs of others
Treat code reviews with the same importance as any other task. Block off regular time slots in your calendar specifically for reviews. Swift feedback is crucial – the longer a developer waits, the hazier the context becomes, making it challenging to incorporate suggestions.
Code reviews aren't solely about catching bugs. They offer an opportunity to familiarize yourself with the team's evolving features and coding principles. Embrace this chance to learn and grow.
Your comments during a review should be lucid and constructive. Vague remarks can lead to confusion. Always aim to provide feedback that guides the developer towards the intended solution.
While platforms like GitHub are fantastic for code hosting and initial reviews, they might not be the best for extended discussions. Consider moving prolonged conversations to a platform like Slack for a more dynamic exchange.
For extensive pull requests, it can be beneficial to pull the branch into your local development environment. Tools like IntelliJ Idea can provide a more immersive diff view. Remember to use commands like git merge -no-commit -no-ff to see the changes as if you made them.
If you find certain parts of the code challenging to grasp, don't hesitate to ask the author in person for clarity. It's better to pause and understand than to provide feedback based on assumptions.
Reviewers Assignment Algorithm
There are different approaches to assigning reviewers automatically to PRs. For example randomly or using a matrix approach. GitHub provides two strategies https://docs.github.com/en/organizations/organizing-members-into-teams/managing-code-review-settings-for-your-team#about-auto-assignment
An alternative approach would be to have a custom strategy where each team member has a list of reviewers from his squad, but also two temporary reviewers from other squads.
Choose the approach which fits your team the best considering its size and feature delivering velocity.
The Speed of Code Reviews
While immediate reviews are ideal, they aren't always feasible. Nevertheless, a golden rule to adhere to is the 24-hour timeframe. Aim to respond to a code review request within one business day, even if it's just an initial assessment. This ensures that the code doesn't linger in review limbo, which could delay subsequent development stages.
By adhering to this rule, it's likely that a typical Change List (CL) undergoes multiple rounds of review within a single day, if necessary. Such quick turnarounds not only streamline the development process but also facilitate dynamic discussions between the reviewer and the developer, leading to a more refined codebase.
How to address the review comments
Instead of solely relying on Github comments, use Slack Github connection to facilitate communication on the PRs. It offers a dynamic platform for real-time interactions, making it easier to clarify doubts, seek explanations, and reach consensus on suggested changes.
If there's a particular comment or issue you plan to address later, leave a TODO tag accompanied by a task number. This ensures that you have a clear roadmap of pending actions and that these tasks aren't overlooked in future development stages.
If a particular code section sparks extended debate, it's crucial to document the decisions made. By adding comments explaining the choices and rationale, you prevent repetitive discussions in the future. It serves as a reference, ensuring that future developers or reviewers understand the context and reasoning behind specific code segments.
Links
Google guide: Code Review Developer Guide
Cisco research: https://static1.smartbear.co/support/media/resources/cc/book/code-review-cisco-case-study.pdf
Microsoft: 30 Proven Code Review Best Practices from Microsoft - Dr. McKayla
More readings on CR: https://blog.palantir.com/code-review-best-practices-19e02780015f
Top comments (6)
Having worked with a lot of different teams with different code qualities, something I like to keep in mind: keep comments appropriate for the level of the code base you are working on.
For example, if the project is a total mess, there are no obvious patterns in use (or multiple abandoned patterns) and there is risky, badly written code all over the place. Accept that you won't be able to fix that in a code review, and that criticising every little thing is just going to demotivate your colleague. Unless you can say something that will make life easier for that developer, or there is an actual bug, staying quiet is a great option.
However if the project is extremely consistent and uses very clear patterns, you never want to allow that to be diluted by merging a PR that introduces structural issues or a competing pattern (unless it's part of an agreed refactor of course). If you see a PR like that you need to ask how that came to be: the developer wasn't on boarded properly, or doesn't understand the pattern and needs help, or simply disagrees and doesn't care. All of those need to be addressed outside of a code review.
This was a fun read and some solid food for thought. Thank you!
I have a singular question. And ultimately it might come from a place of nativity. So heads up, if it's that. Would it be counterproductive to consider individuals from other squads (teams) reviewing code if it's meant to speed up the process? As individuals would then have to familiarize themselves with other's work. Versus an internal individual with a general understanding due to stand-ups and codebase/project knowledge?
That's the only part that has me scratching my head.
I've seen that on largish code bases where the squads are pretty loosely defined and everyone is working in the same repo, and it works pretty well IMO, increases shared knowledge of the project a little. Although you are right, it's harder to review a PR for a feature you know nothing about.
It could be more difficult if the teams are essentially working on two completely separate projects though, a few potential problems to look out for maybe:
But if Team A are a client for Team 1s API then their input could be very valuable, so I guess it really depends. Try it for a few weeks and then review!
Eric, thanks for the response! I'm not in the field yet, so consider this more so reconnaissance. Figured all knowledge is good knowledge. The top two issues pointed out for team based operations were actually ones that I thought might be issues even on a round-robin level. As we all have our secret favorites on our team, and our personal preferences for how we like to be interacted with. So there's got to be some silent celebrations or frustrations being assigned to "the nice guy" or "the hard ass." Hahaha!
Now here's another probable-softball for you. Do code bases tend to have review-specs? Metrics code reviews must hit in order to be considered fulfilled? Because I feel like being more explicit about these things could clear up some confusion. Let me know if you've got the time, and thanks again!
Personality issues are seriously underestimated in software dev I think. A happy and secure team learns to write good code together. Insecure, pushy developers covering up for their imposter syndrome can totally destroy a project 🤷♂️
Great point about the review-specs, the more you can automate expectations the better (take a look at Danger JS for example). And yes for things that can't be automated, ideally there is a set of agreed conventions / patterns / decisions that can be referred to somewhere on a wiki say, to avoid extended discussions about things that already got decided previously
Heyyy, thanks for the response back! Isn't it funny how some individuals want exceptional treatment pertaining to themselves and are absolutely deaf when it comes to others. Oh, life! I desperately seek the ability to be a part of a team. I know I'm too green currently, but I really look forward to it down the line. I think it'll be a blast, but if it's not at least I'll learn something.
I'm taking all these pointers and putting them in the brain bin. I've been letting my Linter yell at me. But I would like to dive more into tests and automation at some point. Probably after I've got a better understand of the next step in the roadmap (absorbing some MERN). I've got Danger JS bookmarked. Thumbed through some of the docs. Would be fun to setup a bot, something new to do. Cheers!