This is one of those "we shouldn't need a blog post about this" topics. But sadly... I really feel that we need a blog post about this. I'm talking about the modern practices surrounding code reviews and pull requests.

A Little Context

I'm old enough to remember code reviews that were done with printouts, or with projectors, or (good gawd) on whiteboards. Not only were these reviews difficult to manage tactically, but they were also highly-impractical with regard to true code quality. Because by the time someone conducted a "formal" code review, it was frequently the case that the code was already, mostly done.

So by the time that the rest of the team was laying eyes on it, we could possibly provide some helpful suggestions for last-minute cleanups. But it was nearly impossible to correct wholesale alterations that should've been implemented in the new module/feature/etc.

Even with the early generation of source control tools (e.g., Visual Source Safe or SVN), it was difficult to really get eyes on the work being done by the rest of the team. And when you did manage to spy their code, it was too-often done after it had already been deployed to production.

It's no exaggeration to say that git represents a quantum leap in this area. Nowadays, you don't have to use git. But most of us do. And I really think that git provided one of the best paradigms for group review of code on an ongoing basis. Even if you're using some other source control tool, the pull/merge request process is probably closely analogous to what we currently have in git.

Code Collaboration Comes Of Age

If you're working in a collaborative-coding environment, the pull request is one of the most powerful tools that you have to ensure that everyone sees (or at least, has a chance to see) what everyone else is submitting. Ideally, some (or all) of your team members are perusing most (or all) of the code before it ever gets merged into a parent branch.

This is powerful. This is good. There's no way I'd want to return to an age when we didn't have such convenient realtime tools to view-and-comment on everyone else's code.

But like all good things, even pull requests have... downsides.

A Forum For Snark And Pedantry

Developers, as a general class of people, are known for being a bit... snarky. If we were amazing "people persons", we wouldn't have chosen a career field where the vast majority of our time is spent staring at screens. With headphones blocking out all ambient noise (and any "real" interaction with other people).

Developers can also be very... opinionated. (SHOCKING!!!) Non-developers don't even understand the Holy Raging Fire that is spawned by something as "trivial" as tabs-vs-spaces. Or leading-commas vs trailing-commas. Or classes-vs-functions. But developers already know these debates well. And they're firmly entrenched in one side or the other. And they're likely to shout you down if you dare submit code that doesn't appeal to their inner preferences.

As much as I embrace the pull request process, I must also admit that I've seen it go horribly wrong. Too many times. When you invite another developer to look at your code, you can almost hear them cracking their knuckles, taking a deep breath, and girding themselves for a Snark Battle.

So, even though I really wish that this post were completely pointless, I've come to realize that I've accumulated my own internal series of "Pull Request Rules". And I really hope that, somewhere out there, maybe a few of you will read these - and take at least some of them to heart.

Rule #1: Linting Is Not A "Nice To Have"

My third post on this site was all about ESLint. (You can read it here: https://dev.to/bytebodger/why-i-hate-eslint-and-how-i-learned-to-love-it-40bh) Specifically, I admitted that I originally hated linting tools. They were like some annoying nanny looking over my shoulder, telling me that my chosen style of coding was somehow "wrong".

Today, I love linting tools. I didn't "come around" to linters because I enjoy (or even agree) with all of the pedantic rules they enforce. I came to rely on them as a critical tool because of the pull request process.

Rule #1: ALL nitpicky details about code styling should be defined/enforced in the linter. If the code you've written doesn't satisfy someone's internal idea of "good coding style", but it passes the linter, the time to argue over this is not during the pull request process.

You see, a good linter should be the first-and-primary defense against any of the Code Nazis on your team who want to hold up your pull request over some pedantic detail of styling.

Does your team want spaces-over-tabs? Then it'd better be enforced in the linter.

Does your team want the opening curly brace on its own new line? Then it'd better be enforced in the linter.

Does your team want case statements indented inside the switch block? Then it'd better be enforced in the linter.

If your team is still trying to "enforce" code-styling standards by policy, and can't be bothered to take the time to implement a shared linter, then... welcome to 2002. Your Nickelback concert t-shirt is waiting patiently for you.

I recently worked on a team that thought they were wayyyyy too busy to implement any linting tools. But they still had their own internal hodgepodge of "coding standards" that they felt compelled to enforce. It didn't matter that their so-called standards weren't even consistent in their own codebase. I'd code something up, submit my pull request, and then they'd hammer it with a slew of stylistic changes that they wanted me to make before it could be merged.

When it comes to stylistic decisions, the question should be dead-simple: Did it pass the linter? If it passed the linter (or if you couldn't be bothered to even implement a linter), then it's an extremely poor (and adversarial) practice to use the pull request process as a haphazard means to enforce your poorly-defined "standards".

In case I'm not being perfectly clear, if you feel compelled to litter someone's pull request with a series of dictates to change coding style, you're being a Pull Request Jerk.

Rule #2: Humility

Too many times, I've seen PR comments where the reviewer confidently proclaims that something is "wrong" or must be changed. Even in the egotistic world of application development, such inviolate proclamations are often flawed. And they rarely foster good esprit de cor amongst your teammates.

Rule #2: If you make a bold statement on someone's pull request, you'd damn-sure better be right. And even then... you probably shouldn't be making such bold statements on someone's pull request.

Have a little humility here. Even for The Most Senior Of Developers, it can be extremely challenging to simply look at snippets of code and truly understand everything that's happening there - or why specific coding choices were made. There's always at least a chance that the reviewer just doesn't fully grasp what's happening in the code.

Consider the difference between these two sample PR comments:

Are you sure that you want to use a regular expression here?  
It seems this might be more efficient with a simple string comparison.

Vs.

The RegEx used here is inefficient.  Change it to a simple string comparison.

The second statement is damn... confident. Arrogant, even. The second statement is more likely to spawn a defensive response from the person who submitted the code.

[NOTE: I'm not even getting into the question of whether-or-not you're right. In the example above, it's entirely possible that the use of RegEx is inefficient. It's also possible that the coder really should change the snippet to use a simple string comparison. But that's not the point.]

Couching your feedback in a little humility will usually get a far better response on the PR. It will foster greater respect between everyone who's reviewing the code (including, but not limited to, the original coder).

And no matter how confident you think you are, there's usually at least some chance that you're actually... wrong. Or, at the very least, there's always a possibility that the original coder had a cogent reason for writing the code the way that they did.

Questions foster discussion. Statements foster debate.

Even in those rare cases where I absolutely know I'm right, I rarely put an inviolate statement on the PR. For example, I've seen PRs where the code simply wouldn't compile. Did I code-shame them by stating that something was broken? No. I usually leave a comment more like this:

It doesn't look to me like the line above will compile.  Am I missing something?

Often, in these scenarios, I know that I'm not missing something. But I've found that simply dropping the comment as a far-more-gentle question usually leads to a much greater sense of ongoing collaboration.

Rule #3: Bulldozing Comments

The whole point of a PR review is to encourage as much group participation in the process as possible. One way to completely undermine that participation is to ignore someone's comment altogether and simply merge the code anyway.

Rule #3: No pull request should ever be merged until all of the comments on the request have been answered.

[NOTE: I'm not saying that every comment/question has to be answered to your satisfaction. In a high-velocity environment, it can sometimes even be acceptable if someone answers your comment/question with something like, "Yeah - this needs to be refactored, but for now, we just need to get it merged so QA can start some of the testing process."]

There are few things more disconcerting than to spend careful time reviewing - and commenting - on someone's PR, only to see some hours later that it was merged without any change being made to the code, and without any answer being placed on your comment. When this happens, I refer to it as bulldozing comments.

Recently, I worked on a team where the tech lead bemoaned the fact that their PR process was "broken" and that most of the devs didn't really participate. After working there for mere weeks, it was painfully obvious why this was the case.

Unless you were one of his "chosen devs", you could carefully inspect/comment/question the request - and it would still get merged anyway. Without any response. And without any changes made to the code. It didn't take long for any new devs on the team to realize that it was a waste of time to bother reviewing anyone else's code. They either slapped a blind approval on it - or they just ignored the PR altogether.

Rule #4: Code Avalanches

The previous rules are all about the people who are reviewing the code. But the submitter also bears responsibility in an effective review process. One of the greatest sins committed by the coder is that of the Code Avalanche.

Rule #4: Pull requests should always be as small as possible.

Let's be honest. We've all been guilty of this on occasion. We're working, on our localhost, on The Next Great Feature. It involves dozens of new files. Or massive changes to existing files. It encompasses many thousands of lines of code. And for various reasons, we've been hoarding it in our own private branch that's never been pushed to anyplace where the other devs can see it.

When we're finally ready to get this behemoth deployed to some lower-level environment, we create The Pull Request From Hell. We add everyone who's ever touched the codebase as an approver. And then we wait anxiously for their rubber-stamp approval.

And let's be frank. When we commit this sin (and this code), we all know, deep down, exactly what we're doing. It's such a massive cognitive dump that no one else can possibly hope to read through everything we've written and provide any meaningful feedback. We're secretly hoping that someone will just be overwhelmed by all changes - and just... approve it.

Simply reading code is hard. It's a big ask to expect anyone to really grok the logic when it's not running in their local environment, but instead it's simply "printed" on the screen. This challenge becomes exponentially more difficult when you're asking someone to review thousands of lines of new/updated code.

When you dump a code avalanche on someone, it's possible that they might be able to spot small imperfections in your code. But it's ridiculous to think that they might be able to provide deeper feedback on whether your overall approach "works" - because they'll be challenged to even understand the broader goals of what your new code is doing.

Rule #5: Time Hostages

If you really care about the code-review process, then you should know that any meaningful review takes time. It shouldn't take days. And it should rarely take hours. But it definitely takes time. How many times have you seen an email/chat message like this?

Hey.  I just submitted this pull request. Can someone please 
review/approve it?  I'm trying to make the automated noon deployment 
to QA.

Then you look down at your watch... and it's 11:55 AM.

Rule #5: It is always the responsibility of the coder/submitter to ensure that the requested approvers have reasonable time to provide a good-faith review of the code.

This is another one of those "we've all been guilty sometimes" issues. I get that. But acknowledging our own complicity doesn't make the problem any better. If you're standing over my shoulder (virtually, or physically) begging for an approval, and my timeline for doing so is minutes, then congratulations. You are, officially, trying to turn me into a Time Hostage.

Of course, there are numerous exceptions to this. If I'm confident that the code I'm "approving" will never, in its current state, make it clear through to production - then, yeah... I might be happy to slap an approval on it. But if this code is truly on the fast-track to be deployed live, then I won't be having any of your desperate last-minute pleas.

Rule #6: Accountability

If there's any part of the PR process that's routinely "broken" - in sooooo many dev shops where I've worked - it's the willful lack of accountability. What do I mean by this??

Well, when you slap an approval on someone else's pull request, you absolutely should consider it to be your own personal endorsement of the code. In the rare cases where something truly "bad" makes its way to production, I've (unfortunately) seen the originating developer thrown right under the proverbial bus. But I've rarely seen anyone ask, "But who approved this code?"

Rule #6: Approvals on pull requests should never be taken for granted. They are a personal statement that at least one other developer reviewed your code - and they stand behind it.

[NOTE: I'm not saying that your "approval" means that you absolutely love the code - or what it's trying to accomplish. But I am saying that any approver should bear at least some culpability for any egregious errors that go unchecked.]

At the risk of lawyering this, I want to point out that there are key differences between "culpability" and "responsibility". If Joe managed to get some horrendous code into production, and it eventually brought down the entire site/app, who is ultimately responsible for that failure? Well, Joe, of course. I'm even OK with the idea that Joe might be disciplined - or, in the worst-case scenario, terminated. But if Bob approved that code, then he should also be held, at least to some degree, as culpable for the failure.

This is a really touchy subject in dev shops. On one hand, none of us wants to bear the brunt of the crappy code that Joe managed to get into production. On the other hand, if the approvers on the pull request are absolutely free of any consequences for their actions, then why should they care about giving any meaningful review to the code before they "approve" it??

I've actually seen this play out - positively and negatively - in live environments. I've been on teams where no one dared to ask, "But wait... who approved this code?" And on those teams, approvals were cheap.

I've also been on teams where approvals did indeed bear some degree of accountability. And you know what? On those teams, it's amazing how much more meaningful the approval process became. When people know that their approvals could, in theory, be scrutinized after-the-fact, those people will magically stop "rubber stamping" pull requests. Those people start paying careful attention to any commit upon which they've placed their endorsement.

If this sounds a bit like a "witch hunt", it's absolutely not meant to be. In the scenario I outlined above, the originator of the crappy code (Joe) should always be the one who bears the greatest consequences of his actions. But it's a dire mistake if Bob is not at least questioned about his approval on that crappy code.

I also want to make it clear that the responsibility I'm referring to should only be leveraged in the case of the most egregious mistakes. Bugs happen. I get that. And no one should be expected to have spied every possible bug in your code simply because they read through it in a pull request.

But if something was submitted (and, god forbid, deployed to production) that qualifies as a blatant error, the approver(s) on that PR should at least be called into question. If they're not, the quality of the entire team will suffer. Because the pull request approval process will have been rendered meaningless.

Rule #7: The Ban Hammer

If you truly believe that a pull request is wrong, then when do you actually go so far as to decline it?

Oh, man...

If you think that pull request accountability is a touchy subject, just wait until you get into all of the political landmines that come with declining them. For many devs, declining someone's pull request grates on a lot of raw nerves. It shouldn't. But let's be honest. It absolutely does.

So how do you navigate that minefield?

Rule #7: Declining a pull request is a last resort. But avoiding that last resort should never be an excuse for an approval.

First, let's acknowledge that there can sometimes be... feelings that are wrapped up in declining someone's PR. In a perfect world, none of us would be so emotionally involved in our code. But this is far from a perfect world. And developer personalities can be... touchy. Declining a pull request often feels, to the requester, like you've slapped an "F" on their work.

Second, let's take stock of all the options you truly have at your disposal.

1. As long as your team isn't inclined to Bulldoze Comments, the commenting process itself is often a solid alternative to outright-declining someone's PR. In the example I listed above, if you've put a comment like, "It doesn't look to me like the line above will compile. Am I missing something?" that should be enough to keep the code from getting merged until it's either "fixed", or you've received greater assurances that it actually does compile. So it's usually poor form to decline someone's PR, assuming that you've notified anyone else on the request that you have a problem with it - and that it should be addressed.
   
   

2. You aren't usually required to participate in the approval process. This can be problematic on smaller teams. But assuming that there are other requested approvers on the PR, it can be perfectly valid to just "sit this one out". I've literally had situations where someone came over and directly asked me to approve a PR, I looked at the code and thought it was horrendous, and I told them, point-blank, that I wouldn't approve the request, but I wasn't going to decline it either. In other words, I was basically telling them that I wouldn't personally put myself on the hook for approving it, but I wasn't going to stand in the way if someone else was comfortable doing so.
   
   

3. Deleting a pull request can be a much better option than declining one. For example, imagine that you've seen an outright compiling error in a pull request. You're certain that it simply won't run if it's deployed. But the requester isn't currently at his desk and you're afraid that, if you just sit back and do nothing, someone else might mistakenly approve the PR and it will be deployed to some other environment. In that case, I've often put a comment on the PR indicating my concern - and then I deleted it. Even though it's been deleted, the original submitter should still receive the email notification for the comment that you left on it before you deleted it. And if, for some reason, they don't receive notification of your comment, they'll probably just come over and ask, "Hey! Why did you delete my pull request???" This may feel like it's no different than declining the request. But in my experience, it's amazing how much more open devs are to finding that their request has been deleted, rather than being declined.
   
   

4. There is no requirement that all communication be encapsulated on the pull request. In our modern world, we sometimes forget this simple fact. We assume that, if Joe created some "super janky" code, we have no choice but to point it out on the pull request. But this can feel, to the submitter, like a form of "code shaming". Sometimes, we feel we must go so far as to decline the request. But there are many times when I've just walked over to Joe's desk, or hit him up on Slack, and said something like, "Hey... I saw your latest PR. But something looks really off to me. Is that really what you meant to submit??" In such scenarios, I've often found that this simple act of personal communication is sooooo much more effective than outright declining the PR. This can also be helpful if you're faced with a Code Avalanche. In those cases, I've sometimes pulled the developer over and said, "Walk me through this."

Conclusion

I'm fairly certain that I've left out some critical points. Heck, this might even spawn a Part 2 at some point.

This post might feel like it's full of "bold" (or even, arrogant) statements. There are many aspects of software development that I honestly feel do not lend themselves to hard-and-fast "rules". But as to the stuff that I've outlined in this article, I feel very strongly about it. It would take a lot to convince me that any of these rules are, in fact, flawed.

But... I've been wrong before (allegedly). What say you???