CHECK Pull Request Review Comments

CHECK is an acronym that represents a certain frame of mind that I use while reviewing Pull Requests (PRs). I have reviewed many PRs over my years of development and I've noticed myself settling into a routine that can generally be summarized by the words Curious, Helpful, Exact, Clear, and Kind (CHECK).

But, what do those words means exactly? In this article I'll unpack each of these terms and expand on what I try to convey for each topic.

Be Curious

If you review someone else's code it's really hard to know why they coded something. What were they thinking during the process, why did they write a piece of code like it is, what kind of friction did they encounter while coding, etc...

So, when you see some code that is unclear, instead of your first instinct being, "Why in the world did you write this?" or "What were you thinking?" try to go about being Curious by asking probing questions.

When code stands out to you, resist being accusatory and making assumptions about the reason for the code. This is a good time to ask probing questions and get clarification about what was going on at the time.

Some examples of what this might look like are the following:

What was the rationale for changing the MIME type? Was it to support transparency?

This looks to be a pretty significant set of logic change for a Pull Request that is focused on adding TypeScript and jest unit tests. Where you fixing a bug that you encountered?

Is the surrounding React fragment necessary here?

The above comments open up a conversation to get more information about why the code was changed by the author. Maybe there is a really good reason that isn't obvious to you about why the change was made.

Be Helpful

Pull Requests are one of many ways to grow as a software engineer. It is a blessing for senior developers to take time and provide helpful comments. Helpful comments can come in several flavors: Explain the Why, Code Examples, and Providing References.

Explain the Why

I think it's very important to convey why I'm asking for a certain change to be made. There are many reasons why the author may have chosen their approach. It could be they copied code they saw somewhere else, it could be this is the only way they've ever seen the problem solved, etc...

Explaining the Why helps the author know that there is another way to solve a problem. Also going one step further and sharing the Pros and Cons of a solution can also helpful for the author. Are you asking for a code change because of a performance issue, is this code using an old API that is deprecated, is this an older pattern of solving a problem that has issues, or does this approach not scale over time, etc.... This is an opportunity to help the author learn from this occasion and build a repertoire of solutions.

NOTE: I encourage authors to take notes from comments that I made to help them in their learning process. I'm fully aware that a developer may make the same mistake multiple times as they learn, but having a set of notes can dramatically help reduce the chance of them writing similar code that they need to change in future reviews.

As a reviewer, it sometimes can be a challenge to communicate why you are asking for a specific change. If this is the case, then it's a great opportunity for you to learn and grow. Feel free to ask your peers for their input as you try to explain your reasons for requesting a code change. I've personally have done this many time.

NOTE: I'm contemplating writing another article geared at training new Pull Request Reviewers on an existing codebase. I have many thoughts about how to get someone up-to-speed to review on their own and how that might impact their coworkers on the team. If that sounds interesting to you please ping me on X @elijahmanor to show your interest

Depending on size of app, sometimes there are multiple ways to do same thing. Often times the preferred pattern we want to follow morphs over time. This is especially true as technology or libraries evolve. I'll often see code copied from an older part of the repo that leverages patterns that we'd rather not propagate throughout the system. Therefore, knowing a bit of history about your codebase is helpful when trying to identify code that is built from older patterns.

Code Examples

When asking a developer to make a specific code change it is often more helpful to provide a small code snippet, than to only explain in words what needs to be done. Ideally, it'd be good to provide both to give context to the request.

An example might look like the following:

Since the map only does 1 thing and the parameters are passed as arguments, your code could be simplified to...

select: ({ members }) => members.map(transformMember),

Providing References

It is helpful to reference ideas or topics that you mention with links to external sources, internal wiki, examples that can be found in the current app, etc... There is only so much information that you can provide in a PR comment. If you can point to a reference explaining more about the topic, that provides the author an avenue to pursue the topic in a deeper way.

Here is an example of what that might look like:

We shouldn't need to manually call jest.clearAllMocks() since our jest.shared.config.js sets clearMocks: true. This setting automatically clears mock calls and is equivalent to calling jest.clearAllMocks() before each test. Please remove this line, thanks! (reference)

As you notice common themes of feedback it could be an opportunity to update internal documentation with more details about this topic. The benefit here could be a reference for members on your team to future Pull Requests or a place for developers to reference for help, direction, or training.

Be Exact

There is a temptation to be very terse in PR comments. We are often very busy and sometimes it is tempting to quickly add a general or vague comment and move on. However, this approach can sometimes suppress healthy discussion or minimize learning opportunities because additional effort wasn't taken. For example, instead of saying "this code is wrong, change it" it would be preferable to take time to be Exact (or specific) about what code you are concerned with and provide clear expectations on how it can be addressed.

Being vague in a comment is not helpful and could lead to frustration by the author. It could leave them questioning what you are asking them to do, which is not a great place to be.

Here are some examples of being specific:

act isn't typically needed when using fireEvent because the React Testing Library wraps those in act behind the scenes (reference). You should be able to remove all act calls from this file.

The file extension for this should be .ts instead of .tsx since it's not a React component. Please update accordingly, thanks!

Be Clear

The intent here is to be Clear in the expectations of your comments. Are you asking the author to make a change in their code based on your comment or is it only a suggestion or a note to think about for another time?

This is where the idea of Conventional Comments comes in handy. I've started using this style of PR comments for a while and I've found that it has helped focus the intent of my communication with the PR author. A tool that has helped me is the Conventional Comments Web Extension.

The format of a comment look something like the following:

<label> [decorations]: <subject>

[discussion]

There are many different types of comments that a reviewer could add to a PR. The labels in this format range from a variety of topics such as: praise, nitpick, suggestion, issue, todo, question, thought, chore, note

Typical decorations that you can tag your comments are: blocking, non-blocking, or if-minor. I like to be pretty Clear in my intent so I make sure to add these decorations so the author knows I'm asking them to do something or if it's more informational.

I'll go through some of the more common types that I tend to use and unpack what they mean and show some examples in ways they could be used.

Praise Comments

Comments don't always have to be about something that needs to change. A Pull Request is a great opportunity to encourage another developer in something that they've changed.

The following are examples of what this might look like...

praise: I like how you refactored this piece of code. It is so much easier to reason about now.

praise: Oh, interesting. I didn't realized that API existed, but it really does solve the problem nicely. Kudos.

Nitpick or Quibble Comments

A nitpick or quibble comment is typically something pretty trivial and doesn't necessarily needs to be changed.

nitpick (non-blocking): inset: 0; is the newfangled way to do this

If the above comment had left out the label and decoration, it could have left the author confused if the change was supposed to be made or not. By adding nitpick (non-blocking) that helps communicate that the change could be made, but it's not required for the review to continue.

Suggestion Comments

A suggestion helps communicate an improvement that you want to share with the author. With this type of label it's important to be clear if this is a blocking request or if this comment is only informational knowledge by providing a non-blocking decoration. Having this decoration helps the author know if you actually want the change to be made or not.

suggestion (non-blocking): I prefer using the ?? syntax in cases like this. It's less error prone in cases of falsey values that you don't want https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing

Issue Comments

These type of labels communicate that there is a problem with the specific code. The issue could be a bug in the logic or a visual bug when rendered on the computer. In this scenario it is helpful to explain the bug and either provide a suggestion to fix the issue or open up the conversation on ways it could be fixed.

issue (blocking): When the modal is closed the focus of the modal doesn't go back to the element that opened it. Could you please add support for the nextFocusId prop so that the focus will be properly changed when the "Create" button is clicked?

TODO Comments

A todo comment is some small change that needs to be made. They typically imply that they are blocking, but I tend to be explicit and provide that decoration for clarity.

todo (blocking): It is preferable to use import type for things that are only types. that allows the transpilier to know it can completely remove that code and it's a good indicator to the dev what it's purpose is for https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html

Question Comments

A question comment can tie into the Curious part of CHECK because it is asking questions to the author. Sometimes I'm not certain about a scenario so a good question can help unpack what is going on and why the author made whatever choice they did. I don't typically add a decoration to a question since I'm asking anything to be done at that moment.

The following is a type of question I might ask:

question: I've seen similar code throughout this PR that does similar things. Would this logic be helpful in a common place that could be shared across the app? Or is each code block slightly different?

The other areas where I saw similar code were:

• slices/folder1/transform1.js
• slices/folder2/transform2.js
• utilities/Component.tsx

Thought or Note Comments

Thought or note comments are helpful to just provide additional commentary about the code or overall PR in general. Much like questions, these aren't blocking so I don't tend to add the decoration. An example of a thought comment could look like the following.

thought: We probably should think about refactoring this away from a class component to a React functional component, but today is not the day for that.

Be Kind

If you are requesting a change, then try to be Kind in the way that you request it. Think how you would want someone to ask you. Don't be in a rush to provide your request, take your time and make it count.

Some assumptions might come up in your head as you add a comment. Please don't assume that the developer was lazy as the reason why they made a certain decision or didn't write the code that you think is best. This is a great time to be empathetic and to encourage a healthy discussion to make the code better.

Here are some examples of what kind comments might look like:

todo (blocking): This is using a hard-coded value for size. Could you please pick a size from our utilitiy helper? (@my-company/utilities). The one that you sould need in this case is sizePx.xsmall.

todo (blocking): Could you please remove these console.log statements?

Conclusion

I hope that you've found CHECK to be a helpful framework for you to think about as you perform your Pull Request Reviews. Have you found another approach that you find works for you and your team? If so, I would love to hear about it. Feel free to reach out to me on X @elijahmanor.

NOTE: An alternative acronym to CHECK could be HICKS to represent Helpful, Inquisitive (instead of Curious), Clear, Kind, and Specific (instead of Exact). The ideas are the same for both, but I wasn't sure which one would be easier to remember.