How I Review Code, Part 2

Reviewing code is tricky. When I’m doing it, I’m trying to achieve a few things at once. In this post, I’d like to document the ways I try to add value via code reviews.

My Background & Biases

This is the second post I’ve written on this subject; here’s the first:

https://jakeworth.com/posts/how-i-review-code/

Since 2022, my approach has shifted from strategic to tactical; I don’t prioritize design as much, which I’ll explain in a moment.

When I started programming, I found myself on a team of very experienced engineers. Some of them did not hesitate to tell me my code was bad. Over time I developed a thick skin about code. I don’t internalize feedback and I’ll entertain respectful challenges almost indefinitely. It’s part of my professional approach and something I think has mostly benefited me. The downside is that I assume everybody looks at code this way.

I also pair-programmed for half a decade as a consultant. Despite the fair criticisms of that practice (it’s hard, slower, contentious, etc.), I think it’s better than code reviews in several ways. Primarily, it’s a code review happening in real time, when changes are easy to make and stakes are low.

Code reviews are not real-time, and that’s why I don’t focus on design anymore in my reviews. When it comes to critiquing how some code works, once the high-level strategy is implemented… that’s pretty late in the game. It’s like arriving at a home inspection and saying “I see you built this house in Santa Fe. I think I might like to live in San Diego, actually.” The foundation is poured and the builder is already building another house. A strategic change at that moment is expensive and rarely worth considering.¹

If you’re on a team where code reviews are the primary tool of quality, I think they really matter. It’s the channel to communicate what’s happening in the codebase, providing chances to teach, learn, share context, and request essential changes.

How I Review Code

When I review code, I write conventional comments that try to accomplish at least one of the following:

• Teach
• Learn
• Share context
• Request essential changes

Let me talk about each briefly.

Teaching Via Code View Comments

On small teams, I’ve sometimes been the most experienced programmer. I want to share what I’ve learned (by making a ton of mistakes)! I want to offer what I received in the beginning of my career: many examples of what right looks like.

Here’s a comment pursing this goal:

Suggestion: adding ‘aria-hidden’ here tells screen readers this icon is decorative.

When all my comments are teaching, I just approve the PR, considering my notes to be information. People on my team notice comments even on approved PR’s. Sometimes I use Github’s ‘suggestions’ feature to make implementing my suggestions low-effort.

Learning Via Code Review Comments

I want to learn from everyone, so occasionally I’ll pose a question like:

Question: I see you converted this to a nested ternary. What do you like about that style?

I know about nested ternaries and I have my opinions, but maybe you’ll teach me something, or convince me to use them more! Yeah, it’s nerdy, and not everyone will engage. But when they do, I learn. And giving people chances to teach is good for the team.

I’ve found that asking questions is the best general format for code review comments. Even when I’m certain that a change is a problem, I’ll often leave a question, such as:

Question: Wouldn’t this need to change at the data layer as well?

Not infrequently, I’m wrong, and I’ve learned that jumping to “this breaks stuff, fix it” doesn’t win friends or influence people.

When all my comments are teaching or attempts to learn, I approve the PR.

Adding Context Via Code Review Comments

Some of my comments add context that would be hard to acquire otherwise. Good documentation, wikis, and well-factored code hopefully codifies institutional knowledge. However, sometimes there’s stuff you just can’t know unless somebody tells you.

For instance, our codebase has SVG’s and React components that render SVGs, as we slowly convert to the latter. It would be tedious to change them all at once, so our practice is to convert the SVGs we are working with, and make sure that new SVGs are in the desired format. Someone opening their first PR can’t know that, and so sharing it in a comment helps them and the team:

Suggestion: convert this to a React component via bin/svgConvert.

When all my comments are teaching, learning, or contextual, I approve the PR.

Requesting Essential Changes Via Code Review Comments

Lastly, I request changes that I know must happen. Perhaps something in the PR breaks an expectation. Or it’s premised on an assumption that I know is wrong. Or it overlooks a significant use case. When requesting changes I try to stick to ones like these, where I know it’s going to break something. It must answer “yes” to the question: “Does this have to change?” Here’s an example:

Issue: the mobile app still uses this API; we can’t remove it until the next mobile release.

What if they say “that doesn’t need to change”, but I’m sure it does? Luckily, this doesn’t happen to me often. The people I work with generally take the time to consider the point I’m trying to make.

When a review contains such requests, I don’t approve and either block it or not, depending on my team’s process.

Conclusion

I think code reviews are pretty hard to give, and receive. I hope this post shows how I approach them and the value I try to provide.

Thank you to Hillel Wayne who copy-edited, not proofread 😊, this post.

• Reviews