About the author
Albert Row is a senior software engineer from the San Francisco Bay Area with over 12 years of experience as an individual contributor, technical lead, architect, open-source contributor, and manager.
Albert has been a certified reviewer on PullRequest since December 2019.
One of the challenges most engineers will run into from time to time is being asked to review code in a system they are unfamiliar with. For reviewers on PullRequest, this is understandably common - we see the delta under review but not the rest of the code on the project. So what should developers look for when they’re reviewing code for a system they’re new to?
There’s actually a great deal of value they can provide because they’re less familiar with it.
The value of a fresh perspective
Programmers have a tendency of getting too close to the problems they see day in and day out and fall victim to anti-patterns or suboptimal methodologies by virtue of “doing what they do.”
As an outsider, you may be able to remind someone of an issue they’ve been putting off dealing with or point out a quick solution to a problem most people in the organization have been struggling with. As a new reviewer, the project has an additive gain of a new mind with a unique set of career-based expertise. Fresh perspective brings new ideas, and in many cases, it’s what a developer or a project needs most.
Think about best practices
The best practices that you know from your experience in the places you’ve worked may not have been shared with developers working on projects in different organizations, and they tend to be project-context agnostic.
Both the level and manner of education of software developers receive is currently highly variable (self-taught, CS degree, coding bootcamp). Your experience may provide guidance to recommend a different and better approach based on what has worked in the past to improve the project and save it from foreseeable pitfalls.
Search for security issues
We all make mistakes that result in security vulnerabilities and issies. This is one of the most valuable investigations that can be made by a reviewer.
By looking at a specific HTTP endpoint’s code, for example, it’s frequently possible to find URL parameters being used in unsafe ways, missing validation, or SQL injection vulnerabilities, just to name a few. As a software developer, these things can be easy to overlook during implementation when you’re focused on creating the functionality, and security issues are always relevant and always important; so as a reviewer security holes are always a good thing to look for.
Optimization opportunities
Look for N+1 queries, inefficient loops, potentially expensive joins, and all the other unoptimized code you can find. If you see something, you may save the software developer a bad time during deployment or money for the organization. Most optimizations are small, local optimizations so these are generally easy to find even if you’re not familiar with the overall system.
Ask insightful questions
If there’s a part of the code you don’t understand, ask! Ask what its goal is, how it’s accomplishing the stated goal of the PR, or why it’s structured the way it is. If you as a reviewer are confused, it’s likely a red flag there’s a chance that the code needs clarification or isn’t really doing what it needs to.
Weigh in on naming
One of the core benefits of good naming is so that project self-documents and new contributors can iterate on existing code quickly, reducing cognitive load and avoiding unexpected, future bugs.
As a newcomer to a codebase, you have an excellent opportunity to see if the program is sufficiently self-documenting. Are the method names clear? Are the variable names indicative of the content of the variable? Can you follow the structure of the code based on what you see in the PR?
Suggesting better naming can be valuable for future editors of the code, including the author when they come back to it in six months!
Read the tests
Frequently tests are as valuable for their documentation potential as they are for verifying the functionality under test. Of course, if there aren’t any tests, you could recommend gently that the author write some. And it’s particularly helpful to provide recommendations on what tests should be written. While you’re there you can suggest additional tests that would make the suite more robust and look for tests with too many assertions.
You’ll also find that frequently tests can be DRY’d up via shared setup/tear-down steps or either fixtures or factories for data loading. There’s a lot of work that goes into making test suites both useful and thorough.
Verify the PR contents match the description
This one may seem obvious, but oftentimes you can catch a number of important issues by virtue of ensuring changes are well-identified in PR titles and descriptions. Is the scope of the PR clear? Is the description useful? Does the code do what the PR description says it does? Are there unrelated changes lumped together?
Especially on large projects with lots of contributors, it’s important that we find ways to document the history of project changes over time. PRs provide a source of truth for that. Give feedback on PRs that are unfocused, have unclear descriptions, or handle multiple disparate issues at once, and you’ll help make the project easier to maintain over time.
Check commit messages
Commit messages can give you insight into what’s being worked on (or not). Sometimes you’ll see something like “Changes the structure of the class to conform to our preferred pattern for Singletons in this project” or you might just get “WIP, fixed it”.
If it’s the former, this is really helpful documentation. If it’s the latter, you can coach the other on the value and format of a good commit message - reminding them that they are read and the extra time spent making a descriptive commit message can help the team catch important issues.
Ask “What if I put a null here?”
One of the most common errors in most programming languages is a missing null check. Even the best engineering teams fall victim to overlooking this. Ask yourself what would happen if each variable or function call returned Null. I bet you’ll find one or two missing checks, or potentially missing checks in most PRs. You might have a false-positive rate, but you’ll help programmers keep this in mind as they work.
Give compliments where they’re due
For all of the things that I see that are wrong when I review code, I see many things that are right.
Sometimes I come across brilliantly written code that is a level above what I would have come up with in the same situation. Sometimes I come across elegance, literate programming, impressive simplicity, and beautiful abstraction. In those cases, props must be given!
As someone new to a codebase, this will also communicate to the code author and team that certain practices and care given align with your own professional experience and broader industry standards. It also gives them peace of mind that you’re paying close attention to understanding the code as a cohesive whole, not just focusing on finding problems.
I keep coming back to programming not because of all the things that are wrong and the constant stream of problems,
but for those moments when I get to see truly elevated work and chat with masters of the craft. Reinforcing those
moments when excellence has been achieved makes me think that maybe there will be more of the same down the line.
Software engineer Illustration by Delesign Graphics on Iconscout
Also be sure to check out:
