5 Ways I Provide Value as a PullRequest Reviewer When I Start Reviewing a New Project

As shown in Figure 1 below, even early reviews can provide value.

Catching Security Vulnerabilities

Fresh eyes are golden here. As a developer with over a decade of experience in the industry, I’ve witnessed the patterns of secure and insecure applications time and time again. Most security vulnerabilities are introduced in updates via pull requests, and many of them can be remediated with minor changes. Most of these issues are a result of simple oversight by the programmer, not a result of lack of knowledge, but in some cases, teams won’t know what they don’t know and many security issues are not obvious (see this article on reviewing code to catch broken access control issues and in a part 2 follow-up). In either case, having an experienced engineer review proposed changes will reduce the risk of security vulnerabilities making their way to production.

Common security vulnerabilities that I find on a regular basis are broken access controls, insecure storage of information, poor encryption practices, and writing of sensitive information (such as PII or credentials) to log files. I’ll often find security issues like this the first time I review code for a team.

Weighing In on Architectural Decisions

Many organizations that leverage PullRequest are solid programming organizations with good leadership and strong engineering practices. Many of them create Architectural Decision Documents outlining their technical plans. Frequently these are written in markdown files and committed to source control. PullRequest reviewers always have ready access to these, and in cases where they’re added or updated via PullRequest, I’m able to weigh in as an objective expert on these architectural directions. Having taken tours of duty through a variety of organizations I am able to share hard-won experience with these teams and give them greater confidence in their architectural direction.

Identifying Performance Bottlenecks

Most of the code I review for is written in Ruby and Python—PullRequest has reviewers specializing in virtually every programming language and framework, this is just my niche. Both of these languages have ORMs that are used frequently in web applications. Both ORMs can create a lot of poorly designed queries when used incorrectly. I catch N+1s, missing indexes, and queries that load too much information to process in memory with regularity. On the front end, reviewers catch poor loading patterns from APIs, and across the stack, there are many types of issues that can slow down a web application. Catching these kinds of issues relies on the code reviewer’s experience and expertise, and less on comprehensive context of the stack, team history, and future plans.

Keeping Code Maintainable

Sometimes there’s an easier, simpler way to accomplish a goal that a programmer isn’t aware of when they first submit a pull request. Many of the comments I’ll post when reviewing for a new team for the first few times involve sharing suggestions for simplifying the code, improving its organization, or otherwise changing its structure to improve maintainability, combat technical debt, and promote long-term health. These are rarely hard-and-fast requirements, but many programmers find them valuable as it helps them both improve the quality of their output and learn new tools for down the road so that the next time they see a similar problem they are able to resolve it more easily. Learning/knowledge sharing is one of the most valuable benefits of code review. It makes for stronger developers, stronger teams, better culture, and better products.

Sharing Best Practices and Lessons Learned

A team that has never dealt with security compliance has a lot to learn about building secure applications. A team that hasn’t worked at scale has a lot to learn about performance. A team that is learning a new language together has a lot to learn about building successful projects in that language. As a PullRequest reviewer, I’m always reviewing in my areas of expertise. This allows me to help guide teams when they don’t have deep experience in-house on particular technologies or could benefit from having input from an objective expert when making important decisions.

Conclusion

As a reviewer for PullRequest, I parachute into applications developed by other organizations and go through pull requests with a fine-toothed comb. I liken it to being a digital smoke jumper. Having full and comprehensive context of a project, attending product meetings, understanding team history, and future plans, assuming ownership, etc., does help provide good code review. But is it a firm requirement? No. I’ve found that my years of experience as a professional have been more instrumental in providing valuable insight and guidance in code review.

In fact, it’s easier for an expert with fresh eyes to spot some issues and anti-patterns the core team has become sensitized to. Not only do I catch major issues frequently when reviewing for teams on PullRequest, but I can also say from experience that when I join a new company as a full-time engineer, I catch the most issues in a codebase within my first three months of employment.

This post was originally published on the PullRequest website. On April 28th, 2022 HackerOne acquired PullRequest to help power developer-first security testing solutions. 

Find post author Will Barrett here.