ScaleReady Blog - Code Reviews

Why Do Code Reviews?

There are many strong reasons for doing code reviews. Top of the list are:

What & When to Review?

Intermediate check-ins to private branches don't need to be reviewed. However, once the code builds and automated checks (such as static analysis, unit tests are passing, code coverage is verified) are complete, but before merging into the repository's mainline branch, code reviews must be completed and the reviewer must be noted. Every such merge needs a review. There is no fuzzy rule about what warrants a review and what doesn't. Trivial changes still need reviews.

Before the Review

Review it yourself first. Go through your diffs. Try to anticipate the questions, confusion, and objections that are going to come up. Have good comments. Improve your variable names and anything else that will improve the readability. Get rid of dead code. Of course, by this time you've already built the code and verified correctness with unit tests and whatever other tools you have to make sure quality is high.

If it is a performance sensitive area of code, verify that performance hasn't regressed. Note the results in your change description.

Be sensitive to changes that are too large for a single review and break them down into incremental changes that can be reviewed in smaller, more consumable chunks. This is really important for maintaining a high quality review and preventing your reviewer from screaming and running off into the hills. If you are being asked to review a change that is too large, don't hesitate to ask for it to be broken down into independent incremental changes. As a simple example, style changes or name changes should be separated into a different change set from anything that is intended to change the behavior. By breaking down large changes into smaller ones, you can also better distribute the code review love so it is not all piled on a single reviewer.

A jira-id must be included at the beginning of commit message. This helps give context for the reviewer as well as anyone looking at the change list later.

Follow commit messages best practices: https://chris.beams.io/posts/git-commit/

All projects must be configured to have at least 1 reviewer before merging – this effectively enforces code review

Finding a Reviewer

Propose one (minimum required for every change) or two (if the change feels high risk) reviews from other developers who:

The team lead will probably want to subscribe to be notified of all CR activity and keep a watch on the list of pending CRs. CRs need to be timely and it can be a serious drag on productivity if changes are sitting around waiting for review. Everyone on the team should give high priority to keeping the pending CRs moving just as they would want if their own changes were on the pending list.

It is important that no one is left out of the code review process, however. Everyone needs exposure to the code and the diverse problem-solving skills on the team. We also can't keep a small number of developers so busy with code reviews that they can't get their own projects done. Spread the love.

Performing Code Reviews

Responsibilities

As a code reviewer, you will be held accountable for the quality of the code just as much as if you were the person that wrote it. If a bug makes it to production, you need to feel the same level of concern as the author.

Etiquette

Empathy

Recognize that writing the code took time, energy, and passion. Empathize with the author and be constructive where it matters most. You are not just trying to make the code better, but also to mentor the author and hopefully learn something yourself. Go in with the knowledge that intentions are good and there may be more context than meets the eye. Dig into assumptions - there might be a good reason you are unaware of for why they didn't do it the way you think it should have been done.

Investment in the Team

New contributors to a code base will take some time to get up to speed on how the team likes to do things. Don't overwhelm with hundreds of code review comments. Sit down and talk through the kinds of things that people tend to care about when reviewing changes. Make sure that onboarding a new contributor is a positively happy experience.

Nitpicks

It's fine to comment on things that don't make or break the code. Just label these comments as nit: so it doesn't distract from the more material comments. If you have a lot of nits to pick, it might be time to sit down with the team and talk about and document some coding standards. Even better if you can automate such standards with a linter.

Thoroughness

Code reviews take time and focus to understand the problem that is being solved, the context of how the code fits into the architecture of the system and of the component, what dependencies the code has, and what other code depends on this code, etc. Understand what happens when a dependency fails explicitly or times out.

Purpose

There's some specific purpose to why this change was made. What was it? Don't assume it from the code. Instead, find out the purpose from the Jira ticket. Using your interpretation of the purpose, decide if the change will accomplish that purpose.

What to Look For

Right amount of abstraction

Too little abstraction makes the code less durable to changing needs over time. Too much abstraction is useless and hard to understand. Until you have more than one use case for a piece of code, don't bother with abstraction "just in case." The only thing worse than abstracting based on one example is abstracting based on no examples.

Failure Modes

Make sure that anything that can possibly fail is scrutinized - Memory allocations, disk I/O, network I/O, service dependencies, etc. can all fail and will. When they do fail, make sure the code:

Of course, this implies that error codes need to be captured and responded to. E.g. Don't allocate a chunk of memory and just start using it without making sure that the pointer isn't nil and handling the error appropriately. Yes, this is going to increase the number of lines of code and therefore decrease the code coverage unless you add test cases that simulate the failure.

Coupling

We want to have our services be as independent from other services as is reasonable. If you are able to avoid taking on a dependency without duplicating effort, take the opportunity. This gives your team more control of its own fate and makes for easier debugging and faster recovery when operational issues arise.

Never take a dependency that is less committed to its SLA than your service needs to be. Ideally, you can negotiate a tighter SLA so you don't have to duplicate effort and go your own way. Recognize that whichever path is chosen is a compromise.

Legibility

How quickly were you able to read and understand what was going on? What would make it easier? You just went through the effort of understanding, so make some suggestions on how it can be easier for the next person to understand. They might be doing so under duress in a P1 incident situation - much better to front-load that required effort now.

Testability & Ambiguity

Sometimes a small change to an interface makes the much easier to test. That is worth something. Are there any ambiguous behaviors where you can't tell what's supposed to happen without checking the code to see what actually happens? If so, the interface needs some improvement!

Operability

Is there instrumentation that will help: