Code Review Spanking Machine
May 8, 2017
|Subject:||[company/team-repo] Sizeable new feature (#561)|
You, the most cantankerous developer in all the land, should never review the pull request immediately. This signals that you are not busy, and therefore unimportant - best to stall as long as possible. If you're feeling benevolent, you can give them the "phone call put on hold to cancel cable internet" treatment. Perhaps you're unusually busy, or a bit on edge, in which case they'll get "Saturday afternoon at an understaffed DMV". Heaven help them if you're irritable or swamped with work - that pull request won't see action until the next World Cup.
Float like a butterfly, sting like an Africanized bee
Proceeding through the yawning maw of the pull request link, you will come upon a verdant meadow of pale green code blocks - pristine and vulnerable, like the soft underbelly of an animal of prey.
This is no occasion to be dainty. When your work is through, the pull request should resemble the unlucky gazelle in National Geographic crocodile footage.
The knights who say "nit"
If you give me six lines written by the hand of the most honest of men, I will find something in them which will hang him. Give me six lines of code written by that same man, I will find something in them which will prevent him from merging.
Spacing and indentation? Formatting? Variable names? Grammar and capitalization in comments? Go hog wild! The art of pedantry is only limited by your imagination.
Prefix any frivolous comments with "nit:" to make known to the author that, although it is a trivial matter, they are to fix it anyway as a token of their subservience to your will.
Phrasing your comments
No points are awarded for coddling whoever wrote the code. Some good comment formats:
- You shall...
- Why must you always…
- I demand that you...
- Only a fool would…
- Unless you want this to break...
- It would be less idiotic to…
- You cannot possibly think that...
- This is a hack.
- Swing and a miss.
Pick out a couple, workshop them, and add some flavor to make them your own. Nothing endears you to your colleagues like twisting the knife in a contentious code review.
Find your inner librarian
For any given code behavior, there's a library for it. If your colleague so much as tries to concatenate two strings inline, scold them for bloating the codebase with such reckless duplication. After all, why accomplish something in three lines of code that can also be accomplished with two lines of code and seven required packages?
They've tested the rest, now test the best
There's always a test case that has yet to be accounted for. If there's not an obvious one, it's fairly easy to invent one under the banner of "100% test coverage".
"What happens if this method is passed a vector of objects that allocate huge amounts of memory on the stack, and also attempt to dereference a null pointer at random intervals? Write a test just in case."
The bottomless documentation pit
No description is specific enough. No comments are pervasive enough. "What is the 'i' variable inside this for loop? Please be more specific". If you're looking to fluff up your comment count, this is the place where you can really surf on very little substance.
Your colleague organized the code exactly how they want it. Jam a stick into their spokes and demand they break up all their modules and classes into smaller pieces. Pick an arbitrary line count maxima. "This file is 1016 lines, they should never exceed 1000. Please spend your afternoon splitting this up."
Ah, the old "Not In My Pull Request" mentality. Your colleague foolishly thinks that you will only comment on code that they are modifying. Extinguish this notion. Cement yourself as the alpha by demanding they fix other people's code inside files they had the audacity to modify. Bonus points if you command them to fix punctuation in other people's comments.
Pigheaded to the very end
All code reviews must come to an end, but not while you have breath in your body. Strive to be the last commenter standing no matter how much backpaddling and hair-splitting it takes. Try and time your additional last-minute comments to arrive just as they are about to merge. A wise software engineer justifies their paycheck by being fussy and dogmatic in code reviews.
The expression of utter relief on your colleague's face as they finally merge the pull request surely stems from their knowledge that the submitted code lives up to your exacting standards. Mission accomplished!