Verifi Software Engineer Taylor Caswell doesn’t try to immediately address every issue present in the code he reviews. Rather, he encourages his teammates to focus on their strengths and move on to the next pull request, so they don’t bottleneck the process. He also takes into account the unique strengths and weaknesses of the developers on his team.
“If you are the domain expert, pay careful attention to the domain,” Caswell said. “If you have a knack for clean code practice, keep your eyes out for that in particular.”
And in instances when disagreements in code structure arise, Caswell brings the team together to talk it through. That way, no one feels pitted against another employee or singled out.
Code reviews require a synchronization among team members, and as such, can occasionally block progress. That's why it's crucial that code reviews are prompt — taking hours, rather than days — and developers are aware of the time commitment involved and willing to plan accordingly.
What best practices does your team follow when doing code reviews?
At Verifi, we use Bitbucket’s pull request feature to facilitate code review. This allows us to assign reviewers, track comment threads on specific areas of code and easily see the code changes in question.
By reviewing code frequently, we prevent the dreaded situation of a code review that feels like it contains the entire world. Frequent review means small, easy reviews. It allows reviewers to give the proper attention to the changes being made.
We don’t want code review to become a bottleneck in our development process. This means that we have to be well-disciplined when it comes to addressing outstanding code review in a timely manner. Providing feedback early in the development process helps us avoid wasted work and wasted time. Additionally, we certainly don’t want anyone sitting around for days waiting for someone to review their code.
Every developer has their own strengths and weaknesses. When reviewing code, take this into account. If you are the domain expert, pay careful attention to the domain. If you have a knack for clean code practice, keep your eyes out for that in particular. I wouldn’t recommend that every developer try and look for everything in a code review. We work in teams for a reason. Do what you do best.
When there’s a difference in opinions about how to write certain code, how does your team handle it?
Differences of opinion are common, normal and even beneficial. Code review brings multiple voices into the conversation around an implementation. When there is a disagreement in a code review between the implementer and another team member, we call an informal meeting and gather all of the developers on the team together. Then, as a team, we discuss the problem and weigh the solutions. Sometimes we’ll pick one of the two options in contention. Other times, we’ll come up with an entirely new solution that exceeds the initial disagreement entirely. By calling the team together, we eliminate the “me against you” mentality that is all too easy to fall into. It is no longer one developer’s opinion against another’s.
And, speaking from experience, people (including myself) are more likely to be comfortable having their initial code changed or proposal overruled if it comes from an inclusive team rather than a single opposing developer or manager.
Avoid comments that feel like personal attacks with a simple trick: leave them in the form of inclusive questions.’’
What advice do you have for fostering a positive code review culture?
Maintaining a positive code review culture is essential to having any form of functioning code review. When things get negative, code review becomes a major source of team dysfunction.
Avoid comments that feel like personal attacks with a simple trick: leave them in the form of inclusive questions. Instead of writing, “This method is too long,” try, “Can we shorten this method?” Additionally, make questions inclusive versus exclusive. For example, ask, “Why would you query the database like that?” versus, “How did we come up with this query?”
In both examples, the first comment could have been completely innocent. But conveying intent through text is difficult. The implementer could easily feel attacked or insulted. By changing the language to a question, we turn commands into conversations and we give the implementer the ability to say “no.” By changing the language to be inclusive, we show that we are on the same team.
As a personal rule, after just one or two exchanges, I take the conversation out of the code review and right to the developer’s desk. Long comment threads can easily blow up emotionally, slow down the review and end up accomplishing nothing. Avoid all of that by simply talking to the other person. It seems obvious but it definitely bears repeating often. Don’t risk being misinterpreted or having unnecessary communication failure.
