This is one of those "we shouldn't need a blog post about this" topics. But sadly... Ireally 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 donewith printouts, orwith 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, mostlydone.

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 youdid manage to spy their code, it was too-often doneafter it had already been deployed to production.

It's no exaggeration to say thatgit represents a quantum leap in this area. Nowadays, you don'thave to usegit. But most of us do. And I really think thatgit 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 ingit.

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 isgood. 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 Iembrace the pull request process, I must also admit that I've seen it gohorribly wrong. Too many times. When you invite another developer to look at your code, you can almosthear 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 Ireally 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 originallyhated linting tools. They were like some annoying nanny looking over my shoulder, telling me thatmy chosen style of coding was somehow "wrong".

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

Rule #1: ALL nitpicky details about codestyling 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 isnot 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 wantcase statements indented inside theswitch block?Then it'd better be enforced in the linter.

If your team is still trying to "enforce" code-styling standardsby 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 implementany 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 standardsweren'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 ofstylistic 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 evenimplement 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 codingstyle, 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 beright. 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 simplylook 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'reright. In the example above, it's entirely possible that the use of RegExis inefficient. It's also possible that the coder reallyshould 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 absolutelyknow I'm right, I rarely put an inviolate statement on the PR. For example, I've seen PRs where the code simply wouldn'tcompile. Did I code-shame them by stating that something wasbroken? 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'mnot 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 shouldever be merged untilall of the comments on the request have been answered.

[NOTE: I'm not saying that every comment/question has to be answeredto 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 asbulldozing 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 arereviewing the code. But thesubmitter 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 beas small as possible.

Let's be honest. We'veall been guilty of this on occasion. We're working, on our localhost, on The Next Great Feature. It involves dozens ofnew files. Or massive changes toexisting files. It encompasses manythousands 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 thiscode), 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 througheverything we've written and provide anymeaningful feedback. We're secretly hoping that someone will just be overwhelmed by all changes - and just...approve it.

Simplyreading code is hard. It's a big ask to expect anyone to reallygrok 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 reviewthousands of lines of new/updated code.

When you dump a code avalanche on someone, it'spossible 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 reviewtakes time. It shouldn't takedays. 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 isalways 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 sois minutes, then congratulations. You are, officially, trying to turn me into a Time Hostage.

Of course, thereare numerous exceptions to this. If I'mconfident 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 beyour 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'verarely seen anyone ask, "But whoapproved this code?"

Rule #6: Approvals on pull requests shouldnever 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'mnot saying that your "approval" means that you absolutelylove the code - or what it's trying to accomplish. But Iam saying that any approver should bear at least someculpability 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 ultimatelyresponsible 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 Bobapproved that code, then he should also be held, at least to some degree, asculpable 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 - positivelyand negatively - in live environments. I've been on teams where no one dared to ask, "But wait... whoapproved this code?" And on those teams, approvals werecheap.

I've also been on teams where approvals did indeed bear some degree of accountability. And you know what? Onthose teams, it's amazing how much moremeaningful 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) shouldalways be the one who bears the greatest consequences of his actions. But it's a dire mistake if Bob is not at leastquestioned about his approval on that crappy code.

I also want to make it clear that theresponsibility 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 theyread through it in a pull request.

But if something was submitted (and, god forbid, deployed to production) that qualifies as a blatanterror, 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 iswrong, then when do you actually go so far as todecline it?

Oh, man...

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

So how do you navigate that minefield?

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

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?" thatshould be enough to keep the code from getting merged until it's either "fixed", or you've received greater assurances that it actuallydoes 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 usuallyrequired to participate in the approval process. This can be problematic on smaller teams. But assuming that there areother 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 directlyasked 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'tpersonally 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 thandeclining one. For example, imagine that you've seen an outright compiling error in a pull request. You're certain that it simply won'trun 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, someoneelse 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, theydon't receive notification of your comment, they'll probably just come over and ask, "Hey! Why did youdelete my pull request???" This may feel like it's no different thandeclining the request. But in my experience, it's amazing how much more open devs are to finding that their request has beendeleted, rather than beingdeclined.
   
   

4. There is no requirement that all communication be encapsulatedon 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 outon 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 todecline the request. But there are many times when I've justwalked 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 thatreally what you meant to submit??" In such scenarios, I've often found that this simple act of personalcommunication is sooooo much more effective than outrightdeclining 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 donot lend themselves to hard-and-fast "rules". But as to the stuff that I've outlined in this article, I feelvery strongly about it. It would take a lot to convince me that any of theserules are, in fact, flawed.

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