I Did a Conference Talk About Code Reviews

5 April 2023

Once a year, my employer (Choice Hotels) runs an internal conference called "Mastery" focused on learning new technologies and coming up with new ideas. One of my goals this year was to do a developer presentation at said conference!

At some point in 2022, our dev leadership opened up the invitation for someone to prepare a training presentation on good code review practices. I volunteered. Here and there I would take a few minutes to do research and jot down my thoughts. Originally I planned on doing it as an ad-hoc developer meeting, but eventually I realized Mastery was coming up and pivoted to making this a full-blown conference presentation instead.

All that said, I wanted to share some thoughts about the things I learned, what I focused on, and my overall experience doing my first large presentation like this!

Objectives

There were two main things I wanted people to get out of my presentation.

To help developers see code reviews as more than just a speed bump in the PR process. That is, there is a reason why code reviews are done, and it's not just to provide a green checkmark on a PR before it is merged.
To walk away with at least one insight that you can apply when you do your next code review. My hope was that even though I covered a myriad of topics, everyone could have at a minimum one thing that they could apply in their day-to-day code review process.

Fundamentals

As part of my research, I looked at written resources about code reviews from Google, GitLab, Stack Overflow, Atlassian, and more. From each, I learned that there are many reasons why we do code reviews. Some of my favorites include:

identifying bugs
increasing code quality
mitigating knowledge siloing
increasing mutual responsibility, collective code ownership, and solidarity
peer pressure (if you know your code is going to be looked at, some people are more inclined to make sure it's in the best state it can be)
teaching opportunity

In addition, code reviews are for everyone, whether they are the most junior or senior engineer on the team. Both have something to gain; for example, the junior may benefit from comments on best practices while the senior may benefit from comments on readability and simplicity. Something I really wanted to drive home is that everyone's brain and eyeballs (i.e. perspective) are unique. Even if you don't think you have anything you could possibly add to a code review, you really do. Your input is important.

I also spent a lot of time covering what I like to call "The Self-Review Before the Review". I believe that doing your due diligence before you even open your Pull Request up for review saves time for everyone. This primarily includes doing a once-over of your own code checking for mistakes or things you forgot to do, but it also includes things like doing some basic tests on your own (not leaving it all to QA), writing a detailed description, including screenshots where applicable, and keeping your PR focused on what it's intended to fix or add. Ultimately, I encouraged people to consider the "reviewability" of their own PRs as a way to prepare them to improve the code review process for all involved.

Finally, one of my favorite things I came across in my research was this Stack Overflow question: "How would you know if you've written readable and easily maintainable code?" The first answer was:

Your peer tells you after reviewing the code. You cannot determine this yourself easily because as the author, you know more than the code says by itself. A computer cannot tell you, for the same reasons that it cannot tell if a painting is art or not. Hence, you need another human - capable of maintaining the software - to look at what you have written and give his or her opinion. The formal name of said process is peer review.

For most simple things, it's probably easy enough to tell if you're writing legible code. But as a codebase garners complexity, having that outside feedback is absolutely critical!

Things to Look For

Before starting a code review, it's a good idea to take a brief pause and examine all of the non-code information given to you. The PR description, mockups, screenshots, acceptance criteria, etc. Consider what you expect to see in the PR. How would you implement the changes? Doing this will give you a better mental picture of the PR as you jump into the review.

After that, dive in. This is by no means a comprehensive list, nor can it be, but here's a condensed list of some of the things I think are good to look for in a code review!

Style: Adherence to style guides, linter/formatter issues, general cleanliness.
Naming: Spelling & grammar, do the token names make sense to you, the reader?
Functionality: Does it look like it'll work? Can you spot potential edge cases, bugs, or race conditions?
Design: Is the simplest and most maintainable approach being used? Can you think of a better way?
Complexity: The code should be understandable and not overly clever or over-engineered.
Domain-specific considerations: Frontend/backend/devops/security/etc related PRs will all have different aspects to consider. Depending on your expertise, make suggestions accordingly.
Security: Be aware of common vulnerabilities, what they look like, and how to avoid them.
Positive reinforcement: Look for and compliment good things that you see in a PR!

During my presentation, I also talked about "Things You Probably Shouldn't Skim". That is, sometimes there are certain files that we all tend to skim over quickly during a review that might be worth more than just a passing glance:

Config files: It's easy to ignore a big package-lock.json change, but maybe it's worth considering why the change is there in the first place? What new packages are being added or changed and does this seem correct? In addition, there are many other types of config files in which a one line change could make a big impact but be easily missed. Do all of these changes look intentional and were they vetted?
Tests: Aside from just ensuring tests are written well and are meaningful, they can also somewhat tell you how the code is supposed to be used and essentially provide in-code documentation.
Documentation/comments: Just because the comments don't do anything when the code is running, doesn't mean you shouldn't read it! Does the documentation/commentary do a good job describing what's going on? Is the grammar correct? Does it look like the comment was accidentally copy-pasta'd when the author used other code as a template?

I've really condensed these items down because I didn't want things to get too wordy, but each could certainly have a blog post of its own. Ultimately, there are many things to look for, but not all of these come up in every PR. One day you'll miss something that on another day you wouldn't have. I have a hard time thinking of what a "perfect" review would be, since there are so many factors that go into each. But I believe that as we hone our skills as developers and reviewers we will gradually see more things and be able to more accurately identify the things that need to be commented on.

Miscellanea

Aside from the more "concrete" things mentioned in the previous section, there are a few more "soft skill" aspects to consider when doing code reviews.

First off, ensuring you have good context on the PR is important. Sometimes reading code on your git repository website of choice really doesn't give you the full picture. Don't hesitate to pull the code into an IDE to see the changes in context. You might also consider running the code to see how it works for yourself. Depending on your team, you might do this already!

Asking questions on PRs is critical as well. If you don't understand what a chunk of code does, ask! Starting these conversations often leads to changes for cleaner code or better wording. I think it's also a good idea to ask clarifying questions when you're in a domain you're not 100% familiar with, or to ask about edge cases the author might not have discovered. In my experience, it's always worth asking, and it feels great when something you brought up was something that the author totally missed or didn't consider, and ends up as an improvement to the PR.

Something else that has really helped me as a reviewer is seeking the opinions of my peers about certain aspects of a PR. For me this usually consists of sending a screenshot of a block of code to a coworker and asking what they think about it. "Do you think this is worth commenting on or am I just being nit-picky?" Or, maybe you've written out a comment on a PR but right before hitting save you feel like your tone might make you sound like a jerk. Check with another coworker to make sure your question or suggestion is coming off the right way! Somewhat related, it's also worth keeping an eye on the comments that other developers make on the PRs you see day-to-day. Seeing their comments might give you ideas on other things you should be looking for.

Be Nice 🙂

This technically should be in "Miscellanea" but it's important enough that it deserves it's own section.

We've all had a PR, perhaps early on in our careers, where we received a bajillion comments or one particular comment that could have been written nicer or more clearly. I truly understand that sometimes a lot of feedback is required on a PR. What I'd like to get across is that in all of these scenarios, just try to have compassion and be kind about it. Not everyone accepts suggestions or correction the same way, and when people aren't feeling attacked for what they've written they'll probably be more agreeable to make the changes you request. On top of that, just being generally agreeable can improve the code review dialogue overall.

Remember to comment on aspects of a PR you think the author has done well! Especially from a mentoring perspective with newer developers, hearing a few good things about their code and not just all corrections goes a long way.

I say all of this not so that you should tiptoe around other developers, afraid to upset them; we're all professionals and should be able to take a modicum of correction from time to time. However, I do think just being nice goes a long way.

What is your takeaway?

If you've made it this far, I'd ask that you introspect a tiny bit and think about what your takeaway from all this is. What's one thing you've read that you could apply in your next code review?

MY Takeaways

That concludes a basic summary of what I covered in my presentation. Overall, I'm pretty pleased with how the talk went. Afterward I got several compliments, saying it was their favorite presentation at the conference or that it really helped them. I was honored that a few people that weren't able to be there live reached out to me and said they'd watched the recording afterward!

If I could change anything, it would be:

There were a couple good points brought up in the Q&A portion that I realized probably could have been covered in my presentation.
It might have been a good idea to get feedback from peers to see if there was anything else I should add (which could have helped with the first point).
The abysmal brown/white/orange slide template we had to use for the presentation. Wasn't the prettiest and I'm pretty sure people were sick of it by the time they got to me, one of the final presenters 😅

Ultimately, my hope is that people learned something from my talk (or that you get something out of this blog post!) and that the recording can be used as a resource in future trainings. Time will tell if it all proves useful!

Thanks for reading!