At a high level, my goal as a Lead Software Engineer is to make sure that the team is creating quality products for our customers where quality can be defined as: “Are we building the right thing and are we building it right?”
In this post, I’ll first look at why code reviews do a great job at ensuring we’re building “the thing” right. From there, I’ll look at how to define what’s important to us in a code review, followed by how we at SentryOne perform code reviews. I’ll wrap up by providing some additional resources and information so that you can start defining (or refining) your own code review process.
Why Code Reviews?
At SentryOne, we consider code reviews to be an integral part of our software development methodology for two reasons.
Reduce Engineering Costs
First, code reviews help us catch defects before testing can happen and way before product is shipped to our customers, which in turn reduces our software development costs. In Steve McConnell’s classic, Code Complete: A Practical Handbook of Software Construction, McConnell produces the following chart describing the multiplying effect of finding a defect (p.29):
In the above chart, if a defect is introduced during the Construction phase (with a cost of $1,000) and is not found until System Testing, then it could cost $10,000 to fix! Now compare that to what would happen if the approach we took to solve a problem (Architecture) introduced a defect, but we didn’t notice an issue until it was released, now it could be up to 100 times more expensive to fix!
Grow and Mentor Other Engineers
Secondly, in order to grow a world class engineering department, we not only need to hire the best talent, but we also need to grow and nurture our engineers. Many companies only hire senior engineers, but we pride ourselves in hiring associate and mid-level engineers and coaching them to be seniors and code reviews are an essential part of that growth. Here at SentryOne, advancing your career isn’t just time on the job, it’s the problems you’ve encountered and fixed, the techniques you’ve learned and your domain knowledge in both performance monitoring and SQL Server. As an example, here’s an example write-up that one of our engineers recently completed as part of their code review:
Even though the defect was trivial in nature, the ramifications of the codebase turned out to be a great teaching opportunity about how to approach this type of problem, a good refactor, and the impact it had on our automated tests.
Our Process
Engineering Process
At SentryOne, we leverage Azure DevOps as our one-stop shop for work items, source code, continuous integration, continuous delivery, and test case management. Our source code is kept in a Git repository where we leverage a trunk-based development methodology for making changes. When an engineer has changes they want to bring into the main repository, they’ll create a pull request and fill out the appropriate template which signals the rest of the team that their changes are ready to go.
Code Review Process
Once a pull request has been created, we have two engineers review the work in question. As part of the review, we’re looking for:
- Quality issues—Does this do what it’s supposed to?
- Maintainability issues—How hard is this going to extend or troubleshoot later down the road?
- Testability—Were we able to wrap automated tests around these changes and if so do these tests make sense?
- Readability—Are things named correctly? Is the engineer’s intent clear?
If something is noticed, we’ll leave a comment in the pull request for the author to address. It’s up to the reviewer and author to work out comments left on the pull request as some of the comments may be closer to a, “Hey, did you know you could do …. and clean this up?” which are optional changes. Other comments may be closer to, “The way this is currently written, it’s hard to write any unit tests. What are your thoughts on this refactor to make it more testable?”
Designing the Process
Defining What’s Important
When designing the code review process, it’s essential to know what’s important and what’s not. At SentryOne, when we make changes to the system, it’s important to know why those changes were needed. Is it because of a defect or are we adding new functionality? In addition, when making changes, it’s important to know what other workflows could be impacted because that would change our verification strategy.
The key thing here is that what’s important to SentryOne may not be important to you, so work with your team to determine what you would consider important to know.
Automating What’s Reasonable
Once you know what’s important, it’s time to determine what things are worth automating and getting that set up. We are human which means mistakes happen, so eliminating (or reducing) points of failure will make your code review process go much smoother.
For example, when we’re performing a code review, we think it’s important that there is a work item associated with it so that we can get additional information about why the work was needed. In our source control repository, it’s easy to setup a policy to enforce that to happen.
Snapshot of an Azure DevOps Pull Request Policy List
Simplify as Much as Possible
When we thought about what was important for our code reviews, we came up with a series of questions that, when answered, would help reviewers understand the changes:
- What is the defect? What should have happened instead?
- What is causing the defect to happen? Is that truly the root cause or a symptom?
- What does the solution look like? Are there any other impacted areas?
- How are we verifying that our solution fixed the problem?
To help our engineers do the right thing, we added the above questions as a pull request template for Azure DevOps so that when we create a pull request, we can start with a template and add additional information as needed.
The following screenshot is an example of choosing a Pull Request template from Azure DevOps:
The following screenshot is an example of a Pull Request after the bug template has been selected:
Wrapping Things Up
In this post, we explored the reasoning behind why you should be performing code reviews from both a product quality perspective and from a training/mentoring opportunity. From there, we took a look at the engineering process at SentryOne and how we designed our code reviews to fit within our process. Finally, we wrapped up looking at how to automate and simplify your code review process so that reviewers can focus on changes, not bookkeeping.
Additional Resources
- Code Complete: A Practical Handbook of Software Construction (2nd Edition) by Steve McConnell
- Peer Reviews in Software: A Practical Guide by Karl E. Wiegers
- Brainstorming About Code Reviews by Geoff Mazeroff
- Code Review is the Manager’s Job by John Barton
- Developer code reviews: 4 mistakes to avoid (TechRepublic)
