We are ruthless on code reviews

April 27, 2016

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!

Regards,

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.

Comments

This guy seems like an ass. Not because of what he's asking of the new employee, but how he chose to communicate those thoughts. It comes off as rude. Code reviews don't actually have to be rude no matter how high your standards are.
I'm not impressed by this. I really prefer to do code reviews without calling them code reviews because people have so many hangups and insecurities built up around them doing them or not doing them.
I'd love to both give and receive this kind of feedback! However, it's very hard to not upset coworkers, who don't have the same set of standards as yourself. But why don't they? And why shouldn't they? After all, someone pays you to do this, so why not try to do it as good as possible?
It is laughable that he quotes Emerson on consistency, but promotes a ridiculous convention regarding ordering of imports.
This article is a fine case study in toxic work environments. Hilariously this article takes the high ground on technical quality and still manages to conjure a 2.2MB homepage. That's not caring about quality, that's called bike shedding. Instead of throwing a tantrum whenever someone new treads into your fragile little project, there are a myriad of alternative reactions possible. Why are formatting rules not programmatically enforced? Why wasn't the patch, pulled down, fixed and then explained what was wrong. Regardless if someone's new to the project, or new to the company - the way you acted will never be fruitful - and that's on you. Toddlers might get away with "meep", cartoon images and throwing public hissy fits; senior staff that should be trying to make people feel welcome and understood definitely cannot.
Every code review is a teaching and learning opportunity. If the reviewer just overwrites the author's work and then offers some explanation, there is no confirmation the explanation was understood and the author has little impetus to ensure they understand. The reviewer gains skill at explaining and idea or technique. The author learns it and will be able to incorporate it going forward. The author also has the opportunity to push back on a comment and help everybody could come to learn a new and better way. Reviews make knowledge ripple through an organization. Reviewers don't own the developer's code and it disrespects the author and destroys their sense of ownership to just overwrite what they produce without consultation. It might be acceptable to make a pull request on their branch if the reviewer is unable to make progress explaining something via comments.
I would never want to work with this guy and I feel sorry for the new employee. A review comment of “Beep!” is utterly useless. Author - you need a lesson in being humble.
Does nobody realize this is satire?
Apparantly not. It was pretty obvious to me.

Add new comment

CAPTCHA
This question is for testing whether or not you are a human visitor and to prevent automated spam submissions.