When AT&T introduced code reviews, the company saw a 14 percent increase in productivity and a 90 percent decrease in defects. As well, Jet Propulsion Laboratories estimates saving about $25,000 per code review by finding and fixing code defects at an early stage.
These are just a couple case examples reported in Steve McConnell’s book “Code Complete” regarding the impact code review has on a business. In addition to saving companies time and money, engineers who engage in code reviews gain professional development.
With pull requests, engineers can flag errors, request changes and streamline a cohesive language between the whole team. Just as writers get better by reading other writers, engineers get better through reading other engineers’ code — even if they disagree with it.
At HouseCanary, a company that uses analytics to predict real estate market trends, trust and respect are at the center of their code reviews. When differences arise, Senior Mobile Engineer Worth Baker said his teammates assume that each developer’s opinion is reasonable. Their ultimate goal, Baker said, is to create the best possible product for users and customers, not to win or be “right” every single time.
We talked to five engineering professionals about how their teams manage code reviews. They shared how they developed their best practices while fostering a positive code review culture.
Documoto’s technology is designed to help equipment manufacturers improve technical publishing accuracy and lead time for equipment identification. In order to operate their software optimally, code reviews are essential to decrease errors that would show up in production. Software Architect Paul Skokan said code reviews are also an opportunity for engineers to transfer knowledge.
What best practices does your team follow when doing code reviews?
Code reviews consist of a few different elements. First and foremost, the code must address the problem described in the ticket. If the code does not, then it must be revisited. Next, we can look into how the problem was handled. Being a company that encompasses the startup philosophy, we must be agile and flexible.
Readability is always valued, meaning code can be shared and understood by your peers. Elegant solutions, while generally requiring fewer lines of code, usually don't hold up to this standard. Code can be handed off at a moment's notice, which means that fluency is critical.
Finally, code reviews can and should be used as teaching opportunities. They give us easy access points between fellow developers, who are able to spread both business knowledge as well as code paradigms without breaking cadence. Code reviews allow Documoto developers to grow as engineers and understand customers' needs.
Code reviews can and should be used as teaching opportunities.’’
When there’s a difference in opinions about how to write specific code, how does your team handle it?
The final decision will always land on the person responsible for the particular ticket. We are a small enough company that everyone knows each other well and code reviews can be very personable. Recently, we had a situation where some of our older code used a non-standard pattern. We had to decide whether to move away from the current paradigm, be consistent or use a standard archetype. In this case, we decided to pivot from the newest code and add comments to address the other code later.
This allowed us to move into QA faster and keep the change scope to a minimum. The key here is that none of our practices are set in stone. Sometimes significant changes are required, and best practices must come first. Other times it is imperative to meet deadlines. We always strive to do the right thing, which means balancing all changes against timetables.
What advice do you have for fostering a positive code review culture?
Code reviews should be an opportunity for transferring knowledge and allowing individuals to grow while meeting customer needs. A big part of code review is allowing everyone to own the code, and ensuring that no one person owns a ticket or a project.
One team member can start a project, and another person can finish it. This creates some objectivity when it comes to writing solutions, which comes into play with code reviews. With a growth mindset, it's important to be flexible in decisions and not dwell on past mistakes but move forward in ways that work for your organization. Know your team and help them foster a sense of growth and accomplishment.
Smartwyre develops software for the commerce of agricultural inputs and services. In order for that software to function, Lead Software Engineer Kevin Walter said it’s important to be kind, not critical when conducting code reviews.
What best practices does your team follow when doing code reviews?
Code reviews should be a collaborative show-and-tell process. The code author steps through their code and provides a brief explanation of each step along the way. The reviewer has a responsibility to listen attentively and ask questions or point out sections of interest. We generally try to avoid jumping to other sections of code that take the process out of the natural flow. We like our code reviews to be conducted in such a way as to feel almost like pair programming. Most importantly, the reviewer is careful not to turn a code review into a performance review. We prefer an environment where developers can expect their mistakes to be met with empathy, positivity and with the goal of improvement for everyone involved.
We like our code reviews to be conducted in such a way as to feel almost like pair programming.”
When there’s a difference in opinions about how to write specific code, how does your team handle it?
Our team strives for consensus as much as possible. It's important that each team member feels their opinion is valued, and is considered in a thoughtful way whenever we're not aligned. In cases where a consensus is difficult, we look for subject matter experts to provide additional guidance. If the likely outcomes of different approaches are more or less the same, it may come down to which team member chooses to take ownership of that particular body of code. Our team values ownership over seniority.
What advice do you have for fostering a positive code review culture?
Be kind, not critical. As detail-oriented professionals, it's often very easy for developers to become hyper-critical during code reviews. Strive to foster positivity, two-way knowledge sharing and camaraderie. When developers aren't dreading the code review process, they often come up with some pretty amazing think-outside-the-box solutions. Coincidentally, when reviewers are free to set aside some of the more dogmatic approaches to code review, they have the opportunity to look at solutions with a more open mind, and perhaps learn a thing or two along the way.
Payfone validates millions of mobile and digital identities, helping companies provide an enhanced customer experience. Jennifer Risdon, senior software engineer, said the main goal of their code reviews is to improve code quality. When a developer knows the code they wrote will be reviewed, it’s typically a driving force to produce well-written code, she explained.
What best practices does your team follow when doing code reviews?
When a developer finishes a development task, he or she submits a pull request, which is available to the entire development team for review. Not everyone has to review at that time, but the PR is always there for developers to go back and refer to when they may have a task similar to what the PR covered. We currently require one approval to merge the code to the branch.
When doing code reviews, we look for consistency and strive to keep our codebase consistent and subscribe to the DRY (don’t repeat yourself) principle of coding. The code should be clean and readable. Also, a PR should only be submitted once it has had the proper unit tests written around it, and the developer has run those unit tests during a self-review of their code.
If any refactoring is done, it should be proven that the code still functions in the same manner and produces the same results.
Mutual respect is key.’’
When there’s a difference in opinions about how to write certain code, how does your team handle it?
Typically, the reviewer and developer will get on a meeting or Slack thread to discuss the code in question. Sometimes, the product owner will get pulled in to clarify requirements, and a decision might then be made about which way the code should be written. Other times, the reviewer may be a senior developer who is more familiar with the codebase and may have knowledge of libraries or abstractions that the developer didn’t know about at the time the code was written. This continuous collaboration between team members effectively helps produce code and serves as a learning opportunity to become more efficient at writing future code.
What advice do you have for fostering a positive code review culture?
Mutual respect is key. Always keep in mind there are several different ways to solve a problem, and code reviews offer an opportunity to share knowledge. Being able to have a say in how things are coded empowers developers and contributes to team culture.
HouseCanary uses data and analytics to predict the future of the U.S. real estate market. To make accurate changes to the company’s software, Senior Mobile Engineer Worth Baker said trust between teammates is necessary when differences arise in code reviews.
What best practices does your team follow when doing code reviews?
On HouseCanary’s mobile team, the high-level goal of our code review process is to ensure that code is bug- and crash-free, readable, maintainable and idiomatic to the platform and language in question. Perhaps more importantly, the core value of our code review process is trust: Trust that the author of the code strived to make reasonable, intelligent choices and trust that the reviewer wants to contribute to the health of the project, and not just to prove that they’re “better” at something.
We attempt to strike a balance between in-depth reviews and deference to the experience of the author. Each line of code is considered and questioned carefully, but with the general assumption that the author’s decisions were made with a more complete perspective than may be available to the reviewer. If you were to observe an ongoing review, you’d see reviewers asking for clarification and perspective before offering advice on logic or code structure, pointing out code that conflicts with established syntactic style, and an attempt to avoid wasting time on “bike-shed” style discussions that wouldn’t improve the project as a whole.
By relying on trust in our team as a whole to produce quality code, we end up with a code review process that is perhaps a bit more fluid than other teams, but that has also produced several stable and maintainable projects.
Like any positive culture, trust and respect need to be at the center of critical processes.’’
When there’s a difference in opinions about how to write certain code, how does your team handle it?
Continuing the theme of trust, when disagreements arise, we start with the assumption that each opinion is at least reasonable from some perspective. And if a difference of opinion can’t be resolved by talking it out, we’re all fine with letting the manager of our team have the final decision-making authority.
Most significant differences in opinion arise before the code review stage, when we’re discussing the architecture of a new feature, or the response to some bug or issue. For example, we recently implemented a feature where, when users searched for a city or ZIP code, we would display the geographic boundary of that place on our app’s map, and restrict the displayed property results to within that boundary. We ran into some performance issues with particularly complex boundaries, and different members of our team favored different solutions.
Both solutions were reasonable, but ultimately we decided to implement the simplification algorithm since it would provide a better and more accurate user experience, and meshed more neatly with some future enhancements on our roadmap.
What advice do you have for fostering a positive code review culture?
Like any positive culture, trust and respect need to be at the center of critical processes. Trust that team members will make reasonable choices and respect those choices when differences arise. With those two values in place, authors as well as reviewers can be encouraged to be opinionated, and to stand up for their decisions, without fear of awkwardness or confrontation. The end goal should always be creating the best possible product for users and customers, not winning or “being right.”
IHS Markit transforms financial data into elegant user experiences. To deliver quality code, Senior Principal and Director of Software Engineer Ryan Smith said respect for the team, their goals and their processes is critical.
What best practices does your team follow when doing code reviews?
An important factor for success is to ensure that there is adequate and relevant information available for code reviewers to effectively review the change. That typically starts with a well-crafted change ticket that will ensure the work is well-justified, described and scoped.
The rules and guidelines for approaching the code review process should be agreed upon and followed by all team members and reviewed periodically.
Always start with an assumption of good intent and pose questions accordingly.’’
When there’s a difference in opinions about how to write specific code, how does your team handle it?
It is important to understand that writing code is a subjective experience and healthy debate is an important part of the process.
We ask our team to justify comments with relevant information or experience but understand that others may have contrasting examples. As well, we seek compromise and if that cannot be achieved then the technical owner or architect is responsible for resolution.
What advice do you have for fostering a positive code review culture?
Like most things in life, a good code review practice starts with respect — a respect for each other, a respect for the process and a respect for the goals of the team.
Always start with an assumption of good intent and pose questions accordingly. The process should be highly valued and form an integral part of a team’s workflow.
