Killer PR Review tips
PR (Pull Request) reviews are one of the most important untaught skills in software development. Here I’m going to show you what I’ve learned in a decade in the industry.
Hex Values
The most important thing about a PR review is that you never let a raw hexadecimal value pass into your codebase. You see, hexadecimal numerals are widely used by computer system designers and programmers because they provide an exact value for a color to be shown on the screen. If we use these without putting them in variables, the next developer will know exactly which color we mean, without having to search the code base to find out what hexadecimal value our variable represents.
Imagine you work for a big company, say McDonalds, one with an important universally recognized set of branded colors.
Imagine your boss Ronald says to you, “Hey, we’re changing #FF0000 to #C8A2C8 because the Grimace is really popular right now. Change every red to purple! RIGHT NOW!”
If you’d been smart and used the same variable for #FF0000, changing the color will take you about 30 seconds to find the variable file, and change the value. If you’re a lazy Lucy, who didn’t put in the time to prepare, you’ll spend a whole 45 seconds to Control + Shift + F in VsCode to find and replace all. By that time, Ronald would have thrown you on your keister and replaced you with an intern.
Linters
A lot of smart alex will say, “Yeah, but couldn’t you just put a bunch of these surface level issues into a lint rule and have eslint automatically fix it, freeing up the developers to focus on important parts of the code that have some bearing on the application?”
NO. Don’t be stupid. You get paid $$$ per hour, are you really going to want that time actually coding? If we have a computer remove unnecessary semicolons from our JavaScript, next they’re going to be wanting to drive our cars.
Personal Style
Can you tell what’s wrong with this block of code? I’ll give your 5 minutes.
It’s only been 4 minutes, but I have stuff to do… The else block isn’t necessary because the function would implicitly return undefined if the condition isn’t met. I don’t like needless returns, and even though this will have zero bearing on the code from a user’s perspective, and a completely inconsequential cost in the app overall, I need to tell this person to change it (or else their code will never get merged into the code base). The team codebase is like passing food around a table, you want to put your fingerprints on as much of it as possible.
“Did I ever tell you that this here [code base] represents a symbol of my individuality and my belief in personal freedom? … by personal freedom, I mean freedom for me to reject your code.” — Nic Cage, Wild at Heart
Running the code
Yeah, don’t do this. Changing branches wastes a lot of time. What you want to do with your PR reviews is just to gloss over them with your eyes and let your brain’s JavaScript engine/C compiler/etc run the code instead. If you see anything that doesn’t look like something you’ve seen before, leave a comment saying “That doesn’t look right” or “Should this do this?”. Sure, you might not know what the new code does reliably, but you don’t have to learn how git stash works. Your coworkers will thank you for it.
Gatekeeping
PR reviews are a great way to repeatedly let more junior members of your team know that you’ve been there longer. The only way to strengthen their code is to have them repeatedly put up their PRs until they’ve uncovered and memorized all of your personal “gotchas” of code styling and been fully “initiated” into the team.
Ambiguity
PR reviews can be a lot of fun, for the reasons I mentioned above. A way to keep the fun going on even longer is to neither approve NOR reject a PR, but just leave a bunch of comments. If you were to reject a PR and say “fix this thing, then I’ll approve” your coworker would have a set of professional expectations to follow. Just toss a joke comment in the PR, a gif, or maybe some vague suggestions that imply that if they rewrite everything completely different you might maybe perhaps possibly I dunno…. something maybe.
Don’t do them
Finally, my number 1 tip. Don’t do PR reviews if you can avoid. Those are for people that are not as busy as you. If you want to impress management and your team, not doing PR reviews shows you how seriously you take your job and your commitment to this code base.
