Meta note: I’m leaving for the airport in a few hours and am going to be off-ish the grid until Saturday, and then seriously off the grid until June 1st or so (on a cruise boat, where internet is $45 per second). I have one post scheduled to go out during that timeframe, but expect it to be pretty quiet here until June, at which time I’ll pick back up with the TDD video series and other posts. Thanks for reading! And now, to your regularly scheduled post:
I was having an interesting email discussion with someone not too long ago, and he made an offhand comment about code reviews that I noticed, identified with, and thought was probably a common experience. In essence, he said that he was yet to see them done well and be anything other than depressing experiences. For me, that’s been the case with most, but not all, code review environments. But before throwing the baby out with the bathwater, I’m hoping to write a how to guide for creating dirty bathwater in the hopes that you’ll read it and then not create dirty bathwater. By which, I mean, not give bad code reviews.
I have an odd personality characteristic that compels me to watch or listen to things over and over again if I find them compelling. That is, you might say, “hey, this song’s pretty cool,” and play it for me. If I really take to it, I pity you, because there’s a chance that I’ll play it over and over for like 45 minutes or so. I always feel sort of like a neurotic weirdo, so I try to play it cool and not do it in front of others too much, but, whatever. I’m getting less and less inclined to conceal my weirdness as I get older. I mention this as a segue to explaining the post title.
When I was younger, I watched the Al Pacino movie, “Scent of a Woman,” and I came to be very fond of the Al Pacino character, Frank. He said a lot of very memorable things in that movie, in kind of a gruff, misanthropic, wolfish, but nice-guy-when-you-get-to-know him way. I won’t go through all the quotes here, but I probably could. The reason I could is that I watch things enough times to commit huge swaths of them to memory, and this was the case with this movie. Indeed, this is the reason for the neurotic replay — I don’t just want to experience things; I want to command them and be able to recall and recite them at will later. Anyway, near the end of the movie, Frank gives this extended, loud, angry soliloquy defending his reluctant protege, who is on the precipice of expulsion from school. During the course of this, when talking about the effect that expelling him would have, he says, “you think you’re merely sending this splendid foot-soldier back home to Oregon with his tail between his legs, but I say you are executing his soul!”
What he’s referring to is that the boarding school is on the verge of expelling the protege for taking what he feels to be a principled stand and refusing to “rat out” a classmate in exchange for favorable treatment. The Frank Slade character is saying, in essence, “by punishing this kid for ethical behavior, you’re jading him and altering the course of his life and interactions with others.” And I think that this sentiment has a parallel in our world of programming: I have seen code reviews, things that should be a collaborative, learning experience, subvert group dynamics and result in negativity and hostility being paid forward. So, if you’re interested in a guide on how to do just that and how to turn optimistic, bright eyed developers into angry, cynical people, look no further, because I’m going to tell you.
Nitpick and bikeshed
I think most of us would agree that there’s nothing like spending an hour or two arguing about whether to use implicit typing or not or whether it’s better to trap a null parameter and throw ArgumentNullException or to just let the NullReferenceException happen organically. So, why not make all of your code reviews like that? You can forget about big picture concerns or pragmatic but philosophical discussions such as code readability and ease of maintenance and opt instead to have heated arguments about the little things. Anyone who had previously viewed code reviews as opportunities for learning and collaboration will quickly come to realize that they’re the place for heated disagreements about subjective opinions on details.
Be condescending and derisive
Pointing out a possible logic error is something that would go on in a healthy code review, but why stop there? If pointing out the error drives the point home, adding an “I can’t believe anyone could miss that” will really light a fire under them not to do it again. Since your code reviews are one-way activities, there’s no chance of someone on the other side of the table noticing your mistakes later, so you might as well let ‘er rip with the insults and really dig into their self confidence. They’ll learn to look forward to the day when they can follow in your footsteps and humiliate junior developers from right where you’re sitting.
Make them interminable
If a little code review is good, a 10 hour marathon has to be great. Make sure you go over every line of code, every config file change, every markup tag, and every automated refactoring. Bonus points if you get things wrong in the history and review things that other developers checked in, taking the one in the review to task for code that he didn’t even write. Punish developers for getting their work finished by making them justify every character they’ve added to the code base as if it were a PhD Thesis defense.
Make the code reviews gate-keeper reviews that have to be “passed”
While you’re on the professor-student theme, you might as well go ahead and implement a policy that code has to “pass review.” That way, it’s not a team of professionals reviewing one another’s work and offering critiques, suggestions and insights, but rather a terrified junior developer trying to figure out if she’s good enough to check in code this release. Why not make every sprint/iteration/release just like taking the SATs and waiting for college admissions decisions for all involved? There’s nothing like wondering existentially if you’re good enough at life to have a future to keep the creative juices flowing freely.
Pass your opinions off as facts
Is it your opinion that static methods are better than instance methods? No, of course not. It’s just a fact. At least, that’s how you should approach code reviews. Think of all of your personal tastes in design patterns, coding approaches, style, etc, and then remove qualifying phrases like “I think that,” “I prefer,” or “In my opinion.” So, “I like pre-pending field names with underscores” becomes “you should pre-pend your field names with underscores” and “I find out parameters confusing” becomes “out parameters are bad.” And whatever you do, never back anything up with evidence or citations. When you tell someone that their methods are too long and they ask “what makes them too long,” don’t cite any studies about the correlation between method length and increased defect counts, just see the point about condescension and derision and say something like, “the fact that I’m the senior programmer here and yours are a lot longer than mine.”
Cover things for which checks could be easily automated
Why spend the day on productive tasks when you could sit in a cramped meeting room and pore exhaustively through the code looking to see if anyone committed the grave sin of using camelCase instead of PascalCase on a method. While you’re at it, count the lines in each method to see if they’re too long. Maybe even compute the cyclomatic complexity by hand and then have arguments over whether this method’s is 4 or 5, if you count the default of the case statement. Sure, there are tools that can do any of those things in seconds, but then you lose out on the human touch whereby junior developers are publicly rebuked for their mistakes instead of learning about them quietly and quickly from an automated tool.
Focus only on the negative
It’s amazing how just a little bit of positive feedback can brighten the whole experience, and there should be plenty from which to choose, since examining others’ code always presents an opportunity to see new ways of doing things. So, because of that, you have to be careful to keep it all negative, all the time. Use a whiteboard or a spreadsheet to list people’s mistakes, and the more, the better. Everyone should walk out of code reviews pretty convinced that they chose the wrong line of work and that they can’t hack it. Lights at the end of the tunnel are for cowards.
I’m hoping it’s pretty obvious that this has been a tongue-in-cheek post, but there’s a serious takeaway here. It’s easy to allow code reviews to become negative, depressing and even political affairs. I’ve heard an awful lot of developers talking about hating them, and I think it’s for a lot of the reasons mentioned here — they’re needlessly adversarial and draconian, and people learn to dread them. I also think that there’s kind of a human nature tendency to “pick on the new kid” and make these things tiered affairs where the council of elders passes judgment and doles out tribal knowledge.
To keep code reviews healthy and beneficial, I believe that a concerted effort is required. I might make a post on this as a separate subject in the future, but some general ideas all revolve around staying positive and respectful. If you see things that could be errors, you don’t need to tell people they’re wrong, usually a “what do you think would happen if I passed null into this method” would suffice because the person will probably say, “oh, I didn’t think of that — I’ll fix it when we’re done here.” Allowing them to solve the problem and propose the improvement is empowering and so, so much better than giving them orders to fix their deficient code.
Look at code reviews not as exams or activities to prove the worth of the code, as-written, but as opportunities for groups of people to work toward better solutions. Instill a collective sense of code ownership instead of different people slamming their visions together and watching sparks fly. Reduce the friction in code reviews by complementing the activity with pair or mob programming. That way, more people are defending the work together than the group picking on a single person, and you also get the benefit of not spending days or weeks building up a defensive attachment to your code.
I have seen studies suggesting that the most effective way to reduce defect counts is not to try really hard or run static analysis tools or even (hold your noses, TDD fans) unit tests. The best way, bang-for-buck, to reduce defect count is through pair inspection. It’s far too valuable a tool simply to toss. We just have to be decent about it and treat one another like human beings.