I came to PullRequest from one of the biggest companies in the world with hundreds of thousands of employees. As PullRequest only had less than twenty core-team employees at the time, this was a bit of a shock. I found it particularly interesting how such a small team seemed to be able to accomplish things that my former team could not. It’s understandable that larger organizations as a whole can’t move at the speed of those which are smaller, but even within my former company, smaller, concentrated teams did not move at a comparable speed.
My primary reason for joining a startup was that I’d have the opportunity to be an active contributor in a shorter chain-of-command for making impactful decisions within the code base around code quality and maintainability. When you have bosses and bosses’ bosses dictating product design, there’s a very noticeable lag in velocity, causing programmers to make sacrifices. What I wasn’t expecting was how much value I’d gain as a software engineer by joining a team that practiced disciplined, on-going code review. This was something I’d figure out after taking the leap to join a startup.
Code review was by and large unpracticed on the teams I was part of prior to PullRequest. In this article, I’ll detail general information on what it is, how it works, and the values I’ve found to be especially noteworthy.
Anatomy of a Code Review
A “code review” starts with a series of changes that are destined for the main code stream of a product. When you have these changes ready to be reviewed by another developer (the reviewer), you submit a “pull request” (GitHub, Bitbucket, Azure DevOps) or a “merge request” (GitLab) depending on the underlying service. This request will come with its own description of the changes and directions for reviewers on how to analyze them with respect to the greater body of work.
Over time, the reviewer and the developer will communicate back and forth. Once both are satisfied, the changes get approved, and they can then be merged into the main code stream.
Why Do Code Review?
Code review seemed like a very foreign concept at the outset as my experience was always just pushing my code whenever it’s ready (and tests are passing). While at my former company, I remember a time when I had a changeset about 4,000 lines long. We had just gotten our Github Enterprise deployment set up so I issued my first pull request and assigned two senior engineers to review it! Then, I waited… for a week. Nobody had reviewed it yet.
I believe there were two main reasons for this. First of all, the changeset was way too large! Reviewing 4,000 lines of somebody else’s source code is a daunting task. Second, and more importantly, our team had no code review culture. We assumed each team member was responsible for only their piece of the project. If changes needed to be made, the “owner” would make them. This created very tall silos of ownership for each division with very little communication or growth between them.
While formal meetings and informal sync-ups are effective ways to communicate the general work or ideas among a team, reviewing and understanding each other’s work directly is the best way to build a fully-collaborative culture. Code review allows for shared ownership of responsibilities, having another set of eyes to check what’s actually coded matches its intent, and transitioning new team members into effective contributors.
Cross-Pollination
Code review naturally creates bridges between these silos and a more unified understanding of the larger project they collectively compose. If an engineer in division A finishes a feature and wants their work in the main project stream, they can issue a pull request. At some point, an engineer in division B gets assigned to review it. The engineer from division B now has an opportunity to better understand division A’s code, and at the same time, they can impart lessons that they themselves learned from working on division B as well as learn things that will benefit division B by being exposed to challenges faced by division A. This helps both engineers, and both divisions, grow and understand the greater code base.
For heavily siloed teams, the departure of an experienced developer can be catastrophic - and not just because intimate knowledge of an important piece of your product is lost; you’ll also need to introduce another engineer into a foreign environment. In my experience, this part of the product either atrophies due to neglect (because the new engineer doesn’t want to touch it) or requires a significant refactoring effort. Much of this hardship can be avoided by requiring that engineers review others’ code, even if it’s in a part of the product that they rarely touch.
A Second Pair of Eyes on Your Code Base
Even if a reviewer is a part of the same division, they can analyze the requirements and the solution to determine if the problem was solved properly. They can also look for best practices that may have been overlooked. It’s a great way to find and resolve issues before the code moves on to the next stage, allowing more of a focus on writing new code rather than focusing on fixing old bugs.
When you’re solving a particularly complex problem, it’s easy to overlook small things that aren’t readily apparent. Our brains cannot hold the entirety of a system in its current context - it must make heuristic sacrifices. Allowing another engineer to enter the fray from a different perspective can only improve this situation because it allows for a second copy of the current system to be applied, finding problems in the initial implementation. In addition, the second engineer only has to spend a fraction of the time to review the code as they didn’t have to come up with the solution themselves.
Even within a large organization, like my previous company, expertise in specific software engineering domains was hard to solve for when requesting a review. We were usually limited to requesting review from our own development team. This leads to homogenous development and a team “doing what they do” just because, historically, that’s how it’s been done. To gain the most value from code review, you need to regularly inject experts with fresh perspectives.
Confidence in Software Development
At PullRequest, new developers typically submit and merge their first change within their starting week. A large part of this is because we have senior engineers reviewing their work to ensure that it adheres to our standards throughout the code review process. This allows newcomers to shake off any jitters and ramp up quickly by contributing their first piece of code. This serves two functions: the new hire gains an understanding of the value in having an external code reviewer looking at their code, and it removes their fear of contributing to the code base.
At my first job out of college, new engineers would always start by writing unit tests for existing code. This would give them an introduction to the product as a whole before they’d be given assignments that affect the main code stream. This is a good idea in theory, but the problem with this approach is that there’s always a huge ramp-up for new engineers as they acquire knowledge. It would sometimes take months before they started making a noticeable impact. This can be remedied by starting the discussion about new code and how it affects the product as a whole early-on.
As the new developer grows over the next few months, they will continue to build on a platform that has been properly vetted for coherence with the rest of the code base. This removes many of the doubts when addressing future bugs and shipping new features. This also empowers the newbie to share their knowledge with others and become a more experienced developer who can now contribute to the code review process.
Can Your Workflow Support a System of Code Review?
By all accounts, yes. Off the top of my head, I can think of a number of reasons I’ve heard for why a particular team would operate under the assumption that they can’t spend the extra time and effort on code review. I’ve listed some of them below; however, the best argument would be to just try it out for a while just to see how it affects your team’s cohesion and the quality of new code that gets introduced.
It Takes Too Long
Yes, code review takes time, but a lack of code quality, readability, unified coding style, and maintainability similarly add to the development process by injecting technical debt into the source code. When that technical debt comes due because of bugs and issues discovered during quality assurance or even in production, teams have to set aside valuable time addressing the issue and, often, re-writing large swathes of the code base. And who’s to say that this round of revisions didn’t inject any additional debt that will need to be addressed in the future. A simple process of peer review will help alleviate long-term issues by affording team members the opportunity to find bugs upfront.
If code reviews are properly sized (150-200 lines max), you get maximum value from the reviewer without injecting an unreasonable time or complexity requirement. It’s also good practice to think of a greater change in terms of steps. First, you lay the foundation - class interfaces and directory structure. The reviewer can then take that foundation and offer insights on how better to organize and name the various pieces of the structure. Subsequent changes slowly fill in the functionality and can be addressed in turn. Due to this, the developer stays only one or two steps ahead of the reviewer and can receive feedback quickly without spending too much of their time without any feedback at all.
Often, this lack of feedback leads to doubt about the current assignment, which in turn, leads to unnecessary delays. As the saying goes, “perfect is the enemy of good”. The sooner you get your ideas out there in the sun, the better. More often than not, the thoughts you have about the next steps or the issues you’ll face with your design are shared by the reviewer. This kind of interaction feeds the development process by giving programmers confidence that they’re on the right track.
Nobody Wants to Do It
This is probably the most common argument, and it’s a fair one. However, there are a lot of things that developers don’t want to do including writing progress reports, attending status meetings, and addressing minor bugs. The code review process can actually replace or supplement some of the above processes as it allows for team members to see for themselves what their peers are working on rather than simply hearing about it during a daily meeting.
At PullRequest, most of our work-related interactions are through code review as GitHub itself is a fantastic tool for communicating thoughts and ideas inline with the source code itself. It’s great to have abstract discussions about code in a conference room with a whiteboard, but when making more fine-tuned arguments, it’s even better when you can see the change in context with the rest of the code base.
The Senior Engineers Are Too Busy
It’s always great to have experienced developers perform code reviews because it fosters a process of mentoring for new developers. This is always beneficial; however, these experienced programmers aren’t always the best reviewers. At PullRequest, we’ve seen many senior engineers with hundreds of thousands of lines of review under their belt find very few issues as part of the code review process. This can be because they’re simply rubber-stamping where they are maybe just skimming the code for surface-level code style issues or even approving it without really looking.
Finding the best reviewers is as important as finding the best software developers. These reviewers can quickly look at a set of changes, figure out how it fits into the code base as a whole, and offer insights beyond surface-level coding standards that most reviewers have to offer. During the code review process, the author isn’t the only one who gets value.
Similar to assigning high schoolers Dickens to read, assigning new developers to review a senior developer’s changes benefits the junior as it exposes them to new concepts, best practices, and tools they otherwise wouldn’t have access to. This kind of pair programming is less about code review and more about sharing knowledge between engineers at different skill and experience levels.
Conclusion
It’s not immediately obvious what benefit a team can get from code review. This holds true for many processes aimed to improve development productivity like adopting Scrum or SAFe methodologies. However, after being in the business for ten years, I can say that I feel much more confident in the work that my team members at PullRequest produce because we have a healthy code review culture. As such, we all share in the responsibility to ensure that our code base is heading in the right direction while still owning our own particular pieces of it.
At PullRequest, we help thousands of developers with code review on a daily basis; these include many teams that already practice peer code review and many that are completely new to the workflow. Every team works differently and as such, the value gained from the benefits of code review discussed in this article varies depending on circumstance. However, the net value companies and software developers gain by practicing code review is always significant.
