8  Code Review

8.1 Why Code Review?

We define code review as a process by which a second person reviews code before it is merged into the main branch of a repository. If that sounds like software engineering to you, you may wonder if code review is suitable for science. Code review is a critical part of the scientific process. It is a way to ensure that code is reproducible and that it is understandable and maintainable by others. Code review is also a way to share knowledge and learn from others.

8.1.1 Code Review Makes Better Science

In software engineering, code review emerged as programmers realized it was much easier and cheaper to fix bugs before a product reached the user. In science, code review is a way to catch bugs before your research reaches the public. Bugs, from a scientific point of view, have the same meaning as in software engineering (i.e., a mistake in the code) and a broader meaning (i.e., an error in the analysis). Code review can help catch both types of bugs.

Code review is also a way to ensure that code is reproducible. A second person who can run your code is a good reproducibility test: it simulates what would happen when someone else, much further removed from the project, tries to run your code. Working with the reviewer to make the code run for them will produce more robust code.

Finally, code review is a way to ensure that code is understandable and maintainable by others. When you are deep into a project, you often are so in tune with the work that very complex things can seem obvious. A second person can help you identify places where this may not be so. A bit of back and forth between the author and the reviewer is natural, as the author explains their code and the reviewer asks questions. When you treat this exchange as a continuous improvement process, you’ll find that your code’s ability to stand on its own improves.

All three of these goals help your most important collaborator: future you, who has no idea what current you was thinking when you wrote that code. No, really, you will forget.

8.1.2 Code Review Makes Better Teams

Code review is inherently collaborative. At times, the life of a researcher can feel isolated and lonely. Code review brings someone else into your work and your way of thinking. That relationship is fertile ground for learning and growth.

Reviewing code is a virtuous cycle of education. More experienced coders can help less experienced ones write more idiomatic code. More experienced scientists can improve the use of specific methodology and other types of domain knowledge. Less experienced coders can help more experienced users learn new paradigms in coding. They also serve as a test that code is understandable. Because of this cycle, junior and senior members of the Lab should participate as both reviewers and reviewees.

8.2 Code Review Guidelines

Note: These guidelines are adapted from Google’s code review guidelines. This document expands the ideas in Google’s process to the research setting. Where possible, we cite Google’s original guidelines using block quotes:

Like this.

Google uses its own tools for managing code and code review, so sometimes you’ll see terms that are a little different. A big one is “CL,” changelist. A CL is Google’s equivalent of a pull request (PR).

8.3 When to Stop for a Code Review

In software engineering, code review is a part of daily life. In science, we don’t always write code every day. Some members of the Lab may go weeks or months without writing code. Finishing an analysis before stopping for a code review is also tempting. In other words, adapting the code review process to science requires some flexibility.

Remember that code review is a continuous improvement process. That means early and often is better. But what does that mean for a research project?

8.3.1 Code Review Gates

In the HPDS Lab, we stop at several vital checkpoints or gates for code review. For a traditional research project, these gates are:

  • Initial data pull (e.g., a SQL query or API call)
  • Descriptive tables and figures
  • Algorithms and related tables
  • Reporting

Your project might not fit this template exactly. For example, if you also have a simulation component to your project, you’ll probably want to stop at several points to review the simulation code. Or, you may have no algorithms in the analysis but a more extensive descriptive analysis. In this case, reviewing the descriptive work all at once is probably too large of a code review to be effective.

The general principle is to decide when you absolutely must stop for a code review. Make as many or as few as feels right between them, but always stop at the gates.

8.3.2 That Said, Prefer Smaller Changes Per Review

Smaller changes are more effective and more satisfying to reviewers and reviewees because they are easier to understand and quicker to review.

Here are some relevant points from Google’s guidelines:

Small, simple CLs are:

  • Reviewed more quickly. It’s easier for a reviewer to find five minutes several times to review small CLs than to set aside a 30 minute block to review one large CL.
  • Reviewed more thoroughly. With large changes, reviewers and authors tend to get frustrated by large volumes of detailed commentary shifting back and forth—sometimes to the point where important points get missed or dropped.
  • Less likely to introduce bugs. Since you’re making fewer changes, it’s easier for you and your reviewer to reason effectively about the impact of the CL and see if a bug has been introduced. …
  • Easier to merge. Working on a large CL takes a long time, so you will have lots of conflicts when you merge, and you will have to merge frequently.
  • Easier to design well. It’s a lot easier to polish the design and code health of a small change than it is to refine all the details of a large change.
  • Less blocking on reviews. Sending self-contained portions of your overall change allows you to continue coding while you wait for your current CL in review. …

There are three good ways to think about the size of your PR: number of lines changed, numbers of files changed, and the scope of the changes.

In software engineering, 100-500 lines of code are often considered the optimal PR size. For research, it depends on the nature of the code. For some types of data cleaning, you may need more lines than this. For some algorithmic problems, you may want to step well before this number of lines. Each review should be digestible for the reviewer. You’ll get a feel for what is too big as you do more code reviews.

Similarly, you should limit the number of files changed or added where reasonable. Sometimes, a change to your project automatically generates many files, e.g., figures for a report. That’s okay. But breaking up the review into smaller pieces is a good idea if you are making changes to many unrelated files.

Finally, keep your changes focused on one area.

In general, the right size for a CL is one self-contained change.

You should be able to describe what the change is succinctly. It’s also okay for such changes to touch different parts of your project. For instance, when it comes time for estimation, you may need to modify your data cleaning code for the process to work. But if you then decide to make unrelated changes to an earlier figure, handle that in a separate PR and review.

As a note, “too small” is rarely a problem in code review. Lean towards smaller reviews.

8.3.3 Code Review Is Progress, Too

Often, it can be challenging to stop and wait on your project. For researchers in particular, where typically a single person leads an analysis, you may feel like you are the only one who can do the work. But remember that code review is part of the work. It’s not a delay in the work. It is the work.

Code review is progress. Take a break and enjoy your project moving forward in peace!

8.4 Pre-Review Checklist

Your code review should meet Lab standards. Before requesting a review, make sure you have done the following:

  • Prefer making a change as a pull request. PRs are more straightforward to review than code on the main branch because they are isolated from other changes. GitHub also has tools to help you review code as a PR.
  • Re-run your code from a blank slate environment. This means running your code in an entirely fresh session so that you know your code matches your results.
  • Follow the code style guide for the language you are using. Prefer an automatic tool to style your code for you.
  • Make sure it is clear how to run the code. This might be related to where to run the code (e.g., GCP) or how to install the project dependencies.

8.5 Requesting a Code Review

8.5.1 Identifying Reviewers

Google has a process called readability reviews. In short, someone with readability credentials is an expert on how Google uses a particular language. Readability helps maintain a consistent style and propagate idiomatic code for a given language. Google also requires reviews from another person familiar with the project and an “owner” of the code.

When identifying a reviewer, use this framework. Who can review your code from the perspective of the language you are using? Who can review your code from the perspective of the domain you are working in? Who can review your code from a methodological standpoint? Usually, you are the owner of your research project, but sometimes, you are contributing to someone else’s code. In that case, make sure they are part of the review process.

For sensitive data, prefer to have a reviewer who has access to the data so they can run the code. See Chapter 4 for details on requirements to access project data. Code reviewers who are not on the associated IRB or cleared for data access cannot run the code directly on the data.

Note: You are probably the person most familiar with your project. That’s okay. The reviewer is there to help you make the code better, so they don’t necessarily need to be an expert.

Ideally, you should pick only one or two reviewers because many can slow down the process. The best way to do this is to identify someone who meets as many of the requirements for your review as possible, e.g., who can help improve both your code and the methodology. If you can’t find someone who meets your requirements, you can ask more than one person to review your code.

If you’re not sure who to ask, start with Malcolm. (Typically, Malcolm is on every IRB and can access data directly.)

It may be that your first choice reviewer is not available to do the review promptly. It’s better to have someone else review the code, although it’s a judgment call. Occasionally, you should wait for a particular person to be available if they serve as a critical reviewer. Talk openly about timelines and expectations with your reviewer.

8.5.2 Writing a Good Description

The description of your code review should be a summary of what you are trying to accomplish.

The first line should be a short, focused summary, while the rest of the description should fill in the details and include any supplemental information a reader needs to understand the changelist holistically. It might include a brief description of the problem that’s being solved and why this is the best approach. If there are any shortcomings to the approach, they should be mentioned. If relevant, include background information such as bug numbers, benchmark results, and links to design documents.

See Google’s documentation for examples of good and bad descriptions.

You may also want to guide reviewers on where to focus their attention.

Polish the description before requesting a review and before merging.

CLs can undergo significant change during review. It can be worthwhile to review a CL description before submitting the CL, to ensure that the description still reflects what the CL does.

8.5.3 Iterating on Code Review

8.5.3.1 Responding to Reviewer Comments

When you receive comments from a reviewer, you should make a change or respond to them. If you don’t understand a comment, ask for clarification. If you disagree with a comment, explain why and discuss it. Occasionally, you may want a third party to help decide on a change.

Be respectful and expect the same from your reviewer. Our goal is to improve each other’s work. Remember that, much like a peer review on a manuscript, the review is about your work, not you. Nevertheless, receiving feedback on a project you’ve worked hard on can be difficult. If you are frustrated, take a break and return to it later. Assume good intent from your reviewer.

Remember, courtesy and respect should always be a first priority.

8.6 Giving a Code Review

The first and most important rule of code review is that you are there to improve the code, not perfect it. If the code review ends with the code in a better state than it started, you have succeeded.

8.6.1 What to Look for in a Code Review

The key things we look for in a code review are correctness, readability, and reproducibility. Keep your eye out for bugs, where the code is hard to understand, and where the code is hard to run.

Some other relevant points from the Google documentation are below.

Approaching a code review:

  • Does the change make sense? Does it have a good description?
  • Look at the most important part of the change first. Is it well-designed overall?
  • Look at the rest of the CL in an appropriate sequence.

Detailed points to review:

In the general case, look at every line of code that you have been assigned to review. Some things like data files, generated code, or large data structures you can scan over sometimes, but don’t scan over a human-written class, function, or block of code and assume that what’s inside of it is okay. Obviously some code deserves more careful scrutiny than other code—that’s a judgment call that you have to make—but you should at least be sure that you understand what all the code is doing. If it’s too hard for you to read the code and this is slowing down the review, then you should let the developer know that and wait for them to clarify it before you try to review it. At Google, we hire great software engineers, and you are one of them. If you can’t understand the code, it’s very likely that other developers won’t either. So you’re also helping future developers understand this code, when you ask the developer to clarify it.

What if it doesn’t make sense for you to review every line? … In these cases, note in a comment which parts you reviewed.

Points to consider:

  • The code is well-designed. …
  • Any parallel programming is done safely.
  • The code isn’t more complex than it needs to be.
  • The developer isn’t implementing things they might need in the future but don’t know they need now. …
  • The developer used clear names for everything.
  • Comments are clear and useful, and mostly explain why instead of what.
  • Code is appropriately documented […]
  • The code conforms to our style guides.

Use GitHub’s comments and suggestion features to speed up the process and put discussions close to the code.

8.6.2 What to Avoid

A fine line exists between making code more idiomatic and making it your code. Avoid the latter. Just because you would do something differently doesn’t mean it’s wrong.

Additionally, you should avoid making changes unrelated to the code review. File a new issue to address in a separate change.

Finally, there is a topic that is usually inappropriate for code review: design of the analysis. Not only is a code review a poor fit for such a discussion, but it also hampers the rigor of the study, e.g., adding in ad-hoc analyses that deviate from the research plan. If an analysis has a plan or pre-registration, it is appropriate to discuss if something in the code is done differently from the plan.

8.7 Timeline and Speed of Review

Discuss your timeline for review with the author. If you cannot review the code promptly, let the author know. Fast code review is essential to maintain both momentum on a project and satisfaction with the review process.

Google’s entire guide on the speed of reviews is excellent. Here are some important points:

  • Prioritize reviews but not at the expense of your own focus time. Try to get to it when you have a break in your focused work.
  • Google recommends a single business day as a turnaround time. This goal is good, but it’s not always possible in a research setting with competing priorities. Nevertheless, make code review an important part of your workday. Discuss the timeline for review with the author. Your pending review should not be blocking their progress.

8.7.1 LGTM

The traditional stamp of approval is “LGTM” – “looks good to me!” However you say it, make a clear statement of approval, even if only through GitHub’s approval mechanism.

8.7.2 Giving Comments and Responding to Reviewee Comments

Per Google’s How to write code review comments:

  • Be kind.
  • Explain your reasoning.
  • Balance giving explicit directions with just pointing out problems and letting the developer decide.
  • Encourage developers to simplify code or add code comments instead of just explaining the complexity to you.

Also, clarify when something is a suggestion or for the author’s information. If you have a suggestion that is out of scope for the review, file a separate issue.

There is also some evidence that women and racial minorities are more likely to experience pushback in code review. Be mindful that your review is helpful and not harmful.

Sometimes, authors may disagree with you.

When a developer disagrees with your suggestion, first take a moment to consider if they are correct. Often, they are closer to the code than you are, and so they might really have a better insight about certain aspects of it. Does their argument make sense? Does it make sense from a code health perspective? If so, let them know that they are right and let the issue drop.

In research, too, the author is often much closer to the code and project than you. Nevertheless, your job is to help the author improve their code, so it’s okay to (politely) advocate for a change you think is important. If you can’t agree, ask a third party to help.

Remember, courtesy and respect should always be a first priority

By the way, remember to point out things you like!

If you see something nice in the CL, tell the developer, especially when they addressed one of your comments in a great way. Code reviews often just focus on mistakes, but they should offer encouragement and appreciation for good practices, as well. It’s sometimes even more valuable, in terms of mentoring, to tell a developer what they did right than to tell them what they did wrong.

8.8 Appendix: Code Review Tools on GitHub

GitHub offers top-notch tools for code review. Use a pull request workflow to manage your code changes to take advantage of them. Here are some recommended articles on tools for code review: