Code Review and your team

Let’s say you’re on a small software development team — either within a department or in a smaller organization. You may or may not be doing code reviews, and you may or may not think they’re important (hint: they are).

As a software engineer and software engineering manager, I think that code reviews are one of the most important and valuable things a development team does.

Types of Code Reviews

Code reviews can range from no reviews to formalized code review meetings with review documentation.

• At the least strenuous, is No Code Reviews. This is exactly what it sounds. Developers commit their code and move on to the next task (Note that this is not to say they do or do not test their code, this is focused entirely on reviewing the content of the code changes).

• In a Self Review culture, developers review their own code when finishing up a feature. This helps catch silly mistakes such as committing files that shouldn’t be versioned and acts as a basic sanity check.

• A step up from this is calling over others to review a developer’s code at their desk before committing it. This is also known as a Desk Check and gives an external perspective to code, but the reviewer is constrained to a short period of time and doesn’t have full freedom to look over the code.

• Increasing in formality, a developer can send code off to others for Peer Review at their leisure. This can be done by E-Mail, a source control system such as GitHub or GitLab, or a formal code review tool such as Upsource.

• Drastically escalating the formality, teams can convene a Code Review Meeting between two or more individuals in which the changes are discussed as a team.

• Finally, at the more extreme levels of formality, a formal Code Walkthrough is conducted where the changes are reviewed line by line and notes are recorded on defects and other issues found.

This is a scale of formality in code reviews with more formality not necessarily being better. The appropriate level of rigor depends on the system and team in question. Some important factors include:

• The magnitude of the change
• The sensitivity of the System / Severity of a Failure
• Skill level of the team
• Knowledge level of the team
• Code Quality in relevant areas
• The maturity level of the product offering
• Size of the user base

Teams don’t necessarily need to stick with the same process for each change. A massive refactoring of business-critical systems likely warrants a heavier degree of scrutiny than fixing a Null Reference Exception.

The Value of Code Reviews

Certainly, the value of code reviews comes from finding bugs, enforcing a common code style, and the very act of knowing a code review will occur increases the quality of code committed, but there are other benefits as well.

Code Reviews improve the quality of your code, but they also improve the quality of your team. By involving others in code review, you spread knowledge of the application to others. If a developer goes out on vacation or is tied up on another project, it’s more likely that reviewers will be able to get involved in that project as well. Additionally, if a bug occurs in production after a release, the developer and also the reviewers have a chance to remember the changes made, so involving others in the process increases the team’s capability to respond to emergent work.

Developers will also pick up on tricks and practices from others by reviewing their code and can incorporate these practices into their own code. More experienced developers will teach younger developers new language features and libraries to make their lives easier and help them continually learn. Additionally, different developers tend to have different focuses. A performance-oriented reviewer is going to give entirely different feedback on code than an architecturally-minded developer or a security or testability-minded developer.

Getting the most out of Code Review

I’ve found a few key takeaways for getting the most out of code review. Most of this is aimed at small to medium-sized software as a service (SaaS) organizations since that is where the bulk of my experience is, but these practices should be beneficial to many other types of organizations.

In no particular order:

For the vast majority of reviews, sending a peer review request on to one or two reviewers is sufficient. For larger and riskier reviews, either the submitter or reviewer could request a more formal review process. Alternatively, a team could have a few hard and fast rules such as:

• If there are over 250 lines modified, it needs an escalated review process
• If certain files or methods are touched, it needs an escalated review process
• If the change is only to application data, it requires a lesser degree of formality

When assigning reviewers, I like to assign a subject matter expert to the review as well as someone either on the more junior side or new to that application. This helps spread the knowledge around and gives a safety net.

When actually reviewing code, I like to look through code in a few passes. First, I’ll look over the code as a whole to look for the files changed, types of changes made, and the overall shape of the file. Next I’ll look at the code again reading for what specific changes are being introduced from a functional standpoint. After that I’ll look at architecture, maintainability, and code styles. I’ll often finish up with testability, performance, and security passes. By forcing myself to look at the code through a series of lenses, I can trick my brain into focusing on things in new lights and finding things I wouldn’t find otherwise.

I like to manage code reviews via merge requests and include details in the description as to:

• The business logic around the change
• Any technical debt, architectural, or testability changes
• Any alternatives that were considered / not gone with
• The developer’s plan for testing the change

That last one is particularly important as a code review should validate or find fault with the developer’s plan for ensuring the code change does not introduce defects. This could be as simple as unit tests included with the change or something like “I tested this screen in IE and Chrome and as guest and authenticated user by working with feature X, Y, and Z”

---

Overall, code review is an extremely useful way of improving both your code and your team and a vital way of spreading knowledge, experience, and continuously improving the quality of your code and skills of your team.

Comments

[wizofaus]

Any recommendations on how to encourage all developers in the team to dedicate time each day to code reviews? As team lead its often frustrating that I spend an hour each day on reviews but then my own code can sit there for days without a review despite constant prompting.

[Isaac Lyman]

"Days" is definitely too long, assuming there isn't a weekend or holiday in there. On my team I generally expect PRs to be reviewed within 1 working day.

A few thoughts:

• Are you working to keep your PRs small and incremental? Giant PRs are daunting to review, even for extremely senior developers. In my experience the time required for a review increases exponentially for each additional line of code in the PR. If I'm going into a dev task I know will be big, I try to split it into two or more parts that I can PR separately.
• How do you respond to feedback on your PRs? If you (or a previous team lead) have a history of being argumentative about suggestions and constructive criticism, that can make it draining for other developers to review your work. You may not even know you're coming off in a negative way; I've made that mistake before. It's a good idea to intentionally be extra positive and deferential with conversations like that.
• Is your team proactive about documentation? On my team we'll commonly go through our own PRs (especially if we feel that they may contain confusing or unfamiliar design decisions) and comment each file for the reviewer's benefit. We also document our plans ahead of time, before writing any code, and discuss them with the team so we know what's coming.
• Is your team under high pressure to perform and meet deadlines? If reviewing a PR for an hour puts them at risk of missing an objective and facing consequences at work, then leaving it for someone else to do is, understandably, their best move.
• Is the rest of your team significantly more junior than you? They may be intimidated by your code, or think that they won't have anything valuable to say, or worry that they'll let an important bug through and be responsible for downtime in production.

I expect that if your team knows they should be reviewing PRs, but aren't, there's a psychological reason for it. And with some honest conversation and sustained change in team culture, it's solvable.

[wizofaus]

Generally I put up a PR that fills all the story requirements, if it's obviously too big, sure I'll break them up. As it is, I'm relied on to review almost everyone else's PRs regardless of size, which obviously takes up a sizeable chunk of my day, so I'm well aware of the danger of excessively big PRs.

As for commenting on my own PRs, yes I do that but I try to avoid it, if something needs explaining and it can't readily be done by writing clearer code, then the explanation is probably better off in the code comments. But if it's explaining, e.g, why I've deleted a bunch of code, then sure.

I wouldn't say our team is under unrealistic pressure, but the main reason we struggle to get things done quickly is often because PRs are waiting for review!

Understand about the "concerned-I'm-not-qualified" problem but I've repeatedly emphasised that it's primarily about having a second pair of eyes go over the code (plus an opportunity to familiarise yourself with the code-base etc.). None of our team are that wet behind the ears anyway, but obviously they have varying levels of experience in certain technologies.

[Isaac Lyman]

Sounds like your ducks are in a row then. Maybe it's just Bystander Effect? Maybe you can schedule a half hour with a specific dev and walk them through your PR, then ask for an approval right then and there.