How to perform code reviews like a senior software engineer
How to perform code reviews like a senior software engineer
Programmers spend a surprising amount of their time peer reviewing other people's code. It's one of the easiest ways a team can improve code quality, and it's often a gating requirement before the code can be merged into the main branch of a codebase. But code reviews certainly aren't foolproof, and there's no guarantee every bug will be caught before the code is merged. So the question then is, if you're required to spend the time peer-reviewing every code change, how can you make sure it's a good use of your time?
Every programmer has their own style for how they review other people's code, so don't expect each person on your team to review your code the same way. Some team members may be more reliable for pointing out bag logic, improvements to the code architecture, or suggestions for more performant code. Some team members may test your code thoroughly on their own local machines before signing off on it. And some team members may approve a pull request without offering any comments or suggestions.
Whatever your code review style may be, there are certain techniques that anyone can use to improve how they read and review other people's code.
As with almost all advice, take these suggestions with a grain of salt. There will certainly be situations where some of these bits of advice don't make sense, so use your best judgement.
Gather context
Ideally the author of the pull request took the time to write up a description of what the changes are, and more importantly, why the changes need to be made, but this isn't always the case though. If you're lucky enough to be reviewing a pull request with a description, make sure to read it. Reviewing a pull request with an understanding of what the change is, and why it's needed will go a long way in helping you ask the right questions, point out incorrect logic, or identify any side effects it may cause in other parts of the system.
It sounds simple—just read the description if there is one—but you'd be surprised by how many developers skip the description and read the code without any context. Doing this will help you avoid adding a comment that is easily answered in the description.
Make multiple passes
When making changes to complex codebases, you'll typically need to modify code in many places. This puts a high cognitive load on the person reviewing the code, because they need to understand the how the underlying system works, and then form a new mental model in their mind about how the proposed code changes work within the existing system.
For larger pull requests, it's much easier to understand the implications of a change if you read through the code multiple times. Don't try to understand it all at once on the first pass. Sometimes you may need to read through the changes a few times to fully understand what the new code is doing, and what the side effects are for the system as a whole.
Wait to comment
As you read through a pull request you'll see blocks of red and green lines indicating code that was added or removed. At some point you'll come across a line that catches your attention. The code may reference a variable, function, or class that you're not familiar with, or it may remove a method that you know is used in other places in the codebase. Whatever questions may come up, try to avoid commenting on the code until you've read through the entire pull request. It's possible you may read another block of code further down the page that explains why some code was added or removed further up the page.
Review the requirements
When making a change to the codebase, it's often tied to a specific ticket in your project management tool, such as JIRA or Asana. If the pull request includes a link to a ticket, take a few minutes to pull it up and review the requirements for the change.
Knowing the context behind the code change helps you understand why certain files or functions need to be modified in the code change. Additionally, it also helps you point out changes that don't need to be made because they're not part of the original requirements in the ticket. Some engineers may try to squeeze multiple changes into a single pull request. Depending on your team culture, this may be acceptable or you may want to recommend splitting the changes into multiple pull requests so they can be tracked or reverted independent of one another.
Lastly, reading the ticket helps you identify any missing requirements that were not met with the current code changes, so you can recommend additional changes be made to meet the full requirements of the ticket.
Consider the impact
Even small changes to a codebase can have wide ranging impacts on other parts of the system. When you're reviewing a code change, try to think about any other parts of the system that may use the same functions or classes that were modified. How will they be affected? Did any interfaces change? How about service APIs or function signatures? The impact from these kinds of changes are usually easy to identify and easy to catch before the code is merged.
The impact from changes to configuration files, or changes to how data is persisted and/or fetched from data stores tend to be harder to identify, because they tend to differ from environment to environment. While you may be running a single instance of an app for local development, it may run in a cluster or a distributed manner in your hosted environments. Likewise, the test data you use for local development may not accurately reflect what the real world data looks like in production. Even though the impact is harder to assess for these kinds of changes, it's important to think about these things when reviewing a code change.
If you're not sure what the impact could be, drop a comment and ask the pull request author if they've considered what the larger impact may be.
Move the discussion if needed
Code reviews are asynchronous by nature. The author creates a pull request and tags people as reviewers. Those reviewers read over the code changes, comment, and approve the changes when it's convenient for them. Discussions around specific lines of code can happen over the course of hours, days, and sometimes weeks. Depending on the nature of the discussion, consider moving the conversation to a video chat or in person meeting to hash out specific details.
If there is a difference of opinions between how a specific piece of code should work, it's usually best to schedule a meeting and settle those differences on the spot. Trying to prove your point, or convince others that one solution is better than another over a comment thread in a pull request is rarely successful. When in doubt, talk it out.
Nitpicking
Sometimes you might come across a variable or function that you think could be named better, or a block of code that can be re-written or formatted for easier readability. There will always be small changes that you want to suggest, but that don't really have any significant impact on the logic or the overall feature.
Suggesting these changes is often referred to as nitpicking, and are usually cosmetic or nice-to-have suggestions. Some nitpicking can be good if you're trying to enforce coding standards that your organization has agreed upon. A good habit when adding nitpick comments is to prefix your comment with "nit:" or "nitpick:" so that the author knows this is just a soft suggestion and it's not required before merging the pull request.
After all, why have standards if you're not going to enforce them, right? But be careful not to go overboard. If you catch yourself adding too many nitpick comments on a PR, that may be a sign that your team is lacking sufficient coding standards. Rather than bombard the pull request author with a bunch of minuscule adjustments to their code, try solving the issue by recommending automated linting tools to your team's development workflow.
Additional Reading
If you found this content useful and you’d like to continue building your soft-skills, check out my book, Junior to Senior, published by Holloway. It’s packed with tons of content just like this to help you focus on the non-technical areas that are necessary for programmers looking to make the jump to a senior software engineering role.
Head over to holloway.com to purchase lifetime digital access today.