We are ruthless on code reviews
Dear New Workiva Teammate,
First of all, welcome to the team! You wouldn't have made it if we didn't think you were outstanding. You are totally outstanding! Don't think for a minute that you're not good enough to be here. The truth is, we're privileged to have you here! You were selected because you are someone who learns quickly, strives for excellence and self-improvement in everything you do, and is driven to be the best. Am I laying it on too thick?
Actually, the reason I'm emailing you is to congratulate you on a job well done! I saw that you opened a pull request for the bug fix ticket "WDESK-79888: Cursor appears in two different places after changing table header while holding ALT."
Congratulations! I pulled down your code and tested it myself—it works most of the time. There are a couple unit tests we could add to catch those broken cases.
It wasn't until after the code review that I noticed you were new here. So I must apologize if I seemed rude. Please don’t take it personally. You see, usually I tell people about this prior to the code review: we are ruthless on code reviews.
I don’t mean we’re mean-spirited. I just mean that we are merciless. You’ll notice that I left the comment “Beep!” on the imports of every file you touched. What I meant was, “Your imports violate our standard convention—we order them by built-ins, then third party, and then project level,” but that was too much to type on every file.
Now, I know there were a couple files there where you didn’t touch imports at all, and I still commented. I apologize, but generally, we clean up the small things in any file we touch. It should have been caught on the initial review. Don’t bother using git blame to find out who originally introduced those out-of-order imports. (Psst, it was me, I’m sorry, and I don’t do that anymore).
I see that in response to one of my comments “What does this do?” you changed the behavior of the function in question. I wasn’t trying to be a jerk, and I don’t actually take issue with your original implementation. I was just curious as to what the function did and had hoped you could have explained it in more detail. If you have a great explanation, maybe just include it as a comment in the code itself? Maybe by changing the variable or function names it would be readable enough, and we could forego adding those extra in-code comments.
I know the Bender gif was obnoxious, but in my defense I thought you were joking when you started debating our code review prerequisite-checking robot on code style. If you’ve got a good case on why you’re breaking a formatting rule, that’s fine, we’re reasonable people and open to making exceptions. After all, "A foolish consistency is the hobgoblin of little minds". Our robot is not quite so flexible and unfortunately does not have much natural language processing power built in (yet). Debating her directly is futile.
Our goal is not to pick apart, show off to, or one-up anyone. We’re not tallying up your mistakes or judging you. The intent is to enhance your peers' programming ability, understand every piece of code we are committing, and raise the bar on what everyone involved considers to be the status quo.
One thing to keep in mind is that our customer base is unique. Our customers work for many of the most successful companies in their respective industries. They are where they are because they are committed to excellence and have great attention to detail. Our customers set the bar high for themselves and the quality of the work they produce, and in turn, they have high expectations of us and our software.
Let me put it this way—the game Dark Souls is excruciatingly difficult. Initially playing this game is beyond frustrating—however, it forces you to hone your skill and play smart. Further, when you master an area, the sense of accomplishment from overcoming even a single monster is much greater than what you would get from beating a level of Candy Crush. So, too, is the sense of accomplishment from sailing through a code review while working on a world-class product.
Because of our commitment to quality, we invite you to be just as ruthless and inquisitive when reviewing your peers’ code. You owe it to your colleagues to review their code quickly, but with a level of ruthlessness befitting a comic book super villain.
Thank you for your contributions, and again, it’s great to have you on the team!
Your New Teammate
P.S. I know this letter only addresses 19 of the 43 comments on your pull request, but I was thinking maybe we could discuss a small refactor that would make the remaining comments obsolete.
About the Author
MacLeod Broad is beyond a job title. He works on software system designs, cross-team initiatives, and writes code as often as time permits.