Code reviews are a chance for developers to publicly point out mistakes in each others’ code, so it’s no wonder the practice can seem scary for new developers. But Lisa Scheibner, engineering team lead at Allstacks, said it’s far scarier to her when one of her pull requests inviting other developers to review her code gets approved without any issues being flagged.
“When I think of bad code reviews, I always just think of large code reviews that don’t receive any comments and just get approvals,” she said. “It makes me a little bit nervous. Maybe the code was fine, but there’s always a level of anxiety, like, ‘Was there something missed?’”
HOW TO GIVE IMPACTFUL CODE REVIEWS
- Review the acceptance criteria. Are the code changes within the terms of the acceptance criteria?
- Pay attention to state management. Logic to manage state can be especially tricky for front-end code.
- Check whether the code uses shared components. It can be easy to forget and re-implement existing code.
- Keep an eye on back-end performance. Slim down database queries and monitor how often the back end is called by the front end.
- Don’t forget about logging and testing. They improve error handling and long-term maintenance.
Reviews serve an important purpose in the software development process — they facilitate communication about code standards, support people’s growth as developers and catch costly mistakes. Software developers shared tips with Built In on what they look for in pull requests to find weaknesses in the code and conduct constructive and impactful code reviews.
Make Sure the Code Fulfills the Acceptance Criteria
It may sound basic, but checking whether code changes fulfill the terms agreed to by customers, product and engineering in the project’s acceptance criteria is one of the most important functions of code reviews. Emma Spencer, a software engineering manager at Policygenius, said she always takes a moment to orient herself with the acceptance criteria and the purpose of the pull request before looking at any of the code.
“I have a checklist in my mind of what I need to make sure exists in that code,” she said. “What are the different features that need to work, functionality that needs to be there or changes being requested?”
Instead of jumping straight into the changes, even something as simple as reading through the pull request description can help focus reviewers on the most important things to look for. Spencer said reviewers usually can benefit from a refresher on the nuances of other developers’ work, even though team members are generally aware of the work being done in other parts of the codebase.
“It might have been a couple of days since I’ve really thought deeply about it,” Spencer said. “So just getting that refresher from their perspective, or just even the description of the task, can be really helpful to remind myself of all the specific details.”
She then quickly scans through all the files included in the pull request, looking at file names to get a sense of the extent of the changes.
“Getting that refresher from their perspective, or just even the description of the task, can be really helpful to remind myself of all the specific details.”
“I can kind of create a mental picture of the scope of the changes,” she said. “That helps me categorize the files too. … If I’m familiar with the code, then I can also tell by looking at those file names what a good place to start is — going top-down and following the flow of code execution.”
If the changes are too extensive or complicated for the reviewer to easily understand, Spencer said it doesn’t hurt to reach out and ask the developer for a quick walkthrough of the code.
“It’s pretty much always worth the time,” she said. “If a code reviewer is asking to be walked through the code, that’s an indicator to me that it’s going to take them a long time to review it — because there’s going to be a lot of exploration they’re doing to figure out the context for the changes.”
Code walkthroughs are popular among the development teams she works with. Usually, both parties stand to gain from walkthroughs: The reviewer saves time and gets a better understanding of the code changes and the developer gains experience explaining their coding decisions and receives a more thoughtful review of their code.
Pulling Down the Code Can Save You Time
Scheibner said she pulls down code changes to her local machine whenever there are large or unexpected changes, like missing chunks of files or significant new additions to the codebase. In those situations, being able to see the changes in a code editor can help reviewers navigate through complex code because it’s much easier to search for code snippets and follow the thread of execution in the familiar environment of the code editor.
“Huge chunks of code can get relocated and sometimes it’s hard to find where it went, especially if you’re in a large pull request, 20 files down,” Scheibner said. “By the time I get there, I will have lost that mental connection to remember that this is what I’m looking for.”
Pulling down the code also gives reviewers the opportunity to see code changes in action. Running the code changes can be especially helpful for front-end developers, said Scheibner, who is a front-end developer. She said the additional step allows developers to immediately see the changes reflected in the user interface.
Check Front-End Code for State Management Problems
State management allows front-end code to be less reliant on the back end and improves performance by cutting down on the number of network calls. It’s useful for front-end development, but also especially error-prone.
“You don’t have to continually ask the same questions like, ‘Who’s my logged in user?’” Scheibner said. “Front-end state management exists to hold that information and keep it until you’re absolutely required to request it again.”
But even small logical errors in the code can cause an application to fall into the incorrect state, which can result in problems with the user interface or even costly errors for the company or its users. Reviewers should always pay special attention to complex logic with state management.
Scheibner said state management problems are great candidates for pulling down code and running it locally. Most times, that’s the easiest way to figure out what’s going on with the state and whether there are problems with its logic.
Encourage Developers to Use Shared Components
Development teams often have shared code that gets reused within projects — such as files of helper functions or shared front-end components. But projects sometimes take a long time and codebases can get complex and sprawling, making it harder for developers to learn about and take advantage of shared code.
Code reviewers should check that code changes are taking advantage of shared libraries and components and prevent developers from introducing new implementations of existing functionalities.
Scheibner shared a recent experience where a designer wasn’t able to quickly make design changes because the user interface component they wanted to tweak was implemented differently on different parts of the website.
“Did we mean to be using different tab structures in different areas?”
“He found that we were doing tabs differently in multiple places in the app, even though we had a tabs component,” Scheibner said. “He sent me a question — ‘Did we mean to be using different tab structures in different areas?’”
They hadn’t meant to and quickly changed the code to use the shared component. Being consistent about using shared code isn’t just about keeping the codebase neat. It’s harder to make changes to the codebase later if developers are not able to change the shared component in a single place — instead, developers would have to hunt down the different implementations of the component and change them one by one, a much more painstaking process.
Check for Frequent Causes of Poor Performance
Performance is another important factor to review. Lydia Gorham, senior back-end software engineer at Fast, said a common issue is making database calls that are much more computationally expensive than necessary to accomplish a given task. Expensive database calls can negatively impact performance, especially if it’s placed at a bad spot in the code, like, say, a loop.
Gorham said SQL’s EXPLAIN keyword is a useful tool for getting information about the efficiency of database queries and recommending actions to make queries more efficient.
“Is it hitting the indices in a way that’s going to be efficient?” she said about the insight offered by the EXPLAIN function. “Is there information here that we should be caching because it’s going to be called very frequently and the state is pretty static?”
Gorham said developers and reviewers for back-end code should be mindful of how front-end code interacts with it, to make the best decisions for performance.
“It’s important for the back end to understand the context in which it’s being called, how frequently the front end is going to call it, the patterns of use,” she said. “That’s generally where most of the latency in any kind of interaction is coming from.”
Use Code Reviews to Spread Knowledge Within Teams
Although tasks such as monitoring and logging are usually not essential functions of applications, code reviewers should still check for them because they’re important to how code functions in the real world.
“You want to feel like you’re really aware of the health of your codebase as a whole,” Spencer said. “It just gives you more confidence in the code to know that you’ll be alerted if something goes wrong and that the code review isn’t the final step in ensuring that it’s ‘good code.’”
“Software engineering is a team sport. It only works if everyone can pop in and understand the code.”
Gorham said it’s also important to check for little details like naming conventions, style guides and bugs in edge cases. All those factors keep the codebase readable, so even developers who work on the project years later will be able to easily comprehend and navigate through the code.
“Software engineering is a team sport,” Gorham said. “It only works if everyone can pop in and understand the code. If the person who wrote it is the only person who understands what’s going on, it’s a really big problem long term.”
Developers making the pull request can also contribute to good code reviews by inviting developers from a mix of experience levels to review their code. Some things, like shared libraries and style conventions, can only be learned through experience dealing with the codebase, so having different developers review code together can spread the learning around.
“It’s good to have somebody who’s more senior who can catch things like that,” Scheibner said. “And then it’s also good to have newer people on your pull request because this is how they learn what the code looks like.”
