About the author

Will Barrett is a Software Engineer, Technical Lead, and Engineering Manager from the San Francisco Bay Area with over 14 years of experience. He’s a Superset PMC Member of The Apache Software Foundation. He’s held staff software engineer and senior engineer roles at Change.org, Entelo, Sqwiggle and Preset.

Will is the author of On Learning to Program, a blog for new Software Engineers entering the industry. Will is also a certified reviewer on PullRequest where he’s caught hundreds of bugs, security issues, and other critical issues for over 40 teams.

Important reviewer traits for providing a great code review include prior knowledge and experience, expertise, background context, attention to detail, and written communication skills. As a reviewer on PullRequest, I need to quickly gain context when I’m reviewing for a project for the first time. But as is the case for any engineer new to a team, some context is gained over time.

In my experience, I’ve found that a great code review relies most on the individual programmer’s prior knowledge and experience, expertise, attention to detail, and written communication skills. Here are 5 ways I provide immediate value as a reviewer when I’m reviewing code for a project that I’m new to:

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 request, 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 pull request, 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 issues of these kinds 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 has 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, I can say from experience that when I join a new company as a full-time engineer I catch the most issues in a code base within my first 3 months of employment.

Find this useful? Be sure to check out these additional articles by Will Barrett:

• How to Catch the Top OWASP 2021 Security Category - Broken Access Control - in Code Review Part 1 and Part 2
• Tips for Migrating to a New Computer for Programmers
• The Top 5 Most Common Security Issues I Discover When Reviewing Code
• Reviewing One’s Own Code
• Handling Code Reviews with Empathy
• What We Can Learn About Code Review From the Apache Superset Project
• What We Can Learn About Code Review From the Apache Spark Project