DaedTech

Stories about Software

By

How to Use a Code Review to Execute Someone’s Soul

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.

ExpertBeginner

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.

The Takeaway

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.

By the way, if you liked this post and you're new here, check out this page as a good place to start for more content that you might enjoy.
31 Comments
Oldest
Newest Most Voted
Inline Feedbacks
View all comments
Jake Brown
Jake Brown
10 years ago

Nice satire. I really like code reviews – whenever I start blogging again, I’ll put in an article on ‘painless productive’ code reviews. Some of the highlights – Reviewer should drive. Reviewer should only spend about 5-15 minutes on the review and just look for stuff they find ‘confusing’ or stuff that looks wonky and needs to be explained. 2 people worked best. More adds noise and makes the reviewee feel defensive. Asynch is fine. Shelve code and have someone look at it ** promptly **, or setup a screen-sharing session with voice chat. Reviewing at a desk is not… Read more »

Christian Fey
10 years ago
Reply to  Jake Brown

I totally agree with your points but would add to the following: “2 people worked best. More adds noise and makes the reviewee feel defensive.” that when the code you touch is changing multiple areas (for example, with video game programming, graphics and game logic code), you should definitely have as many people as there are different areas of expertise. A front-end coder, for example, may not realize that there are implications to their rendering code that the gameplay guy will have a heart attack seeing (or simply knows will cause problems). So I’d go more with one reviewer per… Read more »

Jake Brown
Jake Brown
10 years ago
Reply to  Christian Fey

I can agree with that. keeping it small is good, but not so small that you lose context in cross-cutting areas. the idea is just to keep it smaller and simpler. …if you’re making a core API change, I like to run an open-invite presentation or point people to the changes to review on their own.

Erik Dietrich
10 years ago
Reply to  Jake Brown

A lot of good points in here, and I’ll probably read through in more detail when I’m back to my normal day to day, but suffice it to say, when you do start blogging again, let me know. I’d like to see your more formal writeup, because “how to do code reviews” is one of the most difficult “soft topics” out there, I think. I’m always looking for good ideas.

Amitai Schlair
10 years ago

My resume used to list “pair kindly” under team-related skills. A good friend and erstwhile coworker commented that he couldn’t imagine what that might mean. Bless his either totally corrupted or totally uncorrupted heart! So I reluctantly changed it to “pair constructively”. But it seems you might agree that unkindness is the easiest (and probably most common) route to being an unconstructive reviewer or pair.

Enjoy gridlessness! I gotta try that.

Erik Dietrich
10 years ago
Reply to  Amitai Schlair

Not quite gridless yet. I’m in Seattle today and Vancouver tomorrow before we get on the boat and truly go dark. We’re off to see the Space Needle, some museums, and a Mariners game today, but I’m catching up with my digital life while waiting for my girlfriend to get ready. I actually like, “pair kindly” and the sentiment that comes along with it, personally, just because it staves off the “tough love” angle where someone thinks that being mean is just what the doctor ordered for the other person. I’ve never found that being mean or snarky is beneficial… Read more »

dave falkner
dave falkner
10 years ago
Reply to  Erik Dietrich

Careful! We took a trip like that a couple of years ago and had to just move out here (Seattle) to stay. It’s been pretty great.

Richard Buckle
Richard Buckle
10 years ago

I would add that asking junior developers to review a senior’s pull request can be great for both team morale and knowledge transfer.

Erik Dietrich
10 years ago
Reply to  Richard Buckle

Very much agreed. I’ve actually asked junior developers to review my code for this very reason — gives them confidence and provides unique perspective that prevents echo chambers (having someone earnestly ask you something like “what’s the point of unit tests” keeps you sharp about defending your practices).

Robby
Robby
10 years ago

Recommended reading: Walkthroughs, Inspections, and Technical Reviews by Daniel Friedman and Gerald Weinberg.

Erik Dietrich
10 years ago
Reply to  Robby

Thanks — will have to take a look!

Not Ben's Fan
Not Ben's Fan
10 years ago

I think you wrote this after observing a “principal engineer” where I used to work; what you suggest *not* doing is exactly how he conducted things. I never truly hated anyone I’ve worked with until him. It’s been well over 6 years since I’ve even seen him but every time I think of him I ask that the Universe repays him for the way he treated us. I truly hope great evils find their way to his door.

James
James
10 years ago
Reply to  Not Ben's Fan

Well, this is an industry with some of the smuggest, biggest a**holes on the planet. I try not to take is personally, it’s the nature of the industry. I totally sabotaged an interview once, because right before it started, I heard this programmer in the next room act so rude, snide, and condescending to a co-worker that I knew I could never work there and not be filled with constant ire and hate for this douchebag.

BengtQ
BengtQ
10 years ago
Reply to  James

Being rude is not necessarily always bad…

Sometimes a person will not react to friendly advice and mild corrections. He will just shrug it off and continue as he did before.

If that happens repeatedly, I think it’s sometimes justified to apply harder measures and harsher language…

Some people will need it, in order for them to react. It depends upon the personality type.

It’s hard to say if this applies to the situation in the room next to your interview, though.

Erik Dietrich
10 years ago
Reply to  Not Ben's Fan

I think you’re far from alone in being able to name someone who made this impression on you, and it’s this exact dynamic that I think we should be trying harder as an industry to avoid. I cannot imagine that anyone who inspires this level of antipathy and loathing in others is doing his or her organization a service.

A
A
10 years ago

If I’m not the guy the discussion was about I’d be eager to meet them. I’m fond of the gatekeeper approach so I thought I’d offer the other perspective. Not particularly good with the writing style but. I’m the new junior dev for a firm that does a lot of, something, and presents it on a website, if I’m lucky I’ve got a little relevant programming experience. First code review was a disaster, the mean senior dev never explained to me like a child, they must be a dick. I mean, what does it matter if the code randomly deletes… Read more »

Erik Dietrich
10 years ago
Reply to  A

I appreciate the counter-perspective, and I will happily concede that there are comically arrogant junior devs out there who could probably use “being taken down a peg or two” for the sake of their own careers, if not the organization’s. I might think on this a bit and revisit it when I’m not in a hotel and half out the door, but here’s what I’ll say off the cuff about it: Junior developers being arrogant, being lazy, checking in bad code, refusing to learn from their mistakes, etc… those are really performance issues in the org/HR sense. In other words,… Read more »

A
A
10 years ago
Reply to  Erik Dietrich

In hindsight most of the experiences that lead to the opening response in this comment chain could’ve benefited from some HR intervention or at least a few reviews early on. It’s difficult to gauge how interact with new staff and can be easy to jump from the extremely friendly to the cold and distant This quote resonates with me in the sense that it keeps me up at night. “the fact that I’m the senior programmer here and yours are a lot longer than mine.” I’m guilty of using this almost to the letter, I feel its use was an… Read more »

trackback

[…] >> How to Use a Code Review to Execute Someone’s Soul […]

Chris Landry
Chris Landry
10 years ago

For the most part, this is a great article. But when you mention things about code style — in particular naming conventions and white space, I tend to disagree, IF there are coding standards already determined and in place. It can be very distracting when every other class in your code base has a different style. I don’t think everyone needs to be a clone of everyone else, but a code base should have a style that allows people to get an instant clarity of the code if they are familiar with that style. Now I don’t think every single… Read more »

trackback

[…]   英文原文:How to Use a Code Review to Execute Someone’s Soul […]

trackback

[…] blog has been quiet for the last 10 days or so and that’s because, as I mentioned previously, I was heading off on vacation. We had a good time; a few days in Seattle, a brief stop in […]

agaace
agaace
9 years ago

Late to comment but boy you hit home. As a female engineer I feel like my team works just like that. Seriously, I’ve had code reviews when someone opened up an English dictionary and looked up the meaning of words like “instrumentation” to make sure I was 100% semantically correct in my variable names. Or argued with me that “validationResult” is not a good variable name, when the result is set when it is an error or warning, but is null when it is a success (“because it’s not a result if it cannot be a success”) – they even… Read more »

Erik Dietrich
9 years ago
Reply to  agaace

I was just on my way back to my hotel from dinner when I got the notification for this comment. Usually, I respond to comments in batches once per day or two, but I read and re-read your comment because I found it… I dunno, haunting, I guess. In any event, I feel like I have to respond or it’ll bother me all night. I think it’s because your first paragraph transported me back to a low point in my career, many years back. The shop I was in at the time was this dysfunctional waterfall shop, and they would… Read more »

agaace
agaace
8 years ago
Reply to  Erik Dietrich

Hi Erik, it’s me again. You have no idea how much your response meant to me. I meant to reply with something like ‘thanks’, to let you know I appreciated it, but ultimately decided to respond when I fix the situation. So my story hopefully no longer haunts you 🙂 It got to the point when I was waking up thinking how much I hated my job and I couldn’t get over it even on weekends. The thought I was leaving on Friday only to come back 48h later to the same place was depressing. I knew I had to… Read more »

Erik Dietrich
8 years ago
Reply to  agaace

Thanks for the follow up, and that’s great to hear! I can definitely understand how, after leaving, things are so much better that staying in the situation seemed surreal. I’ve had that experience, too. I’m glad to hear that you had such immediate validation of your skills on the open market and were rewarded with a better gig, even in spite of the absurdity baked into the US sponsorship process. It’s been my experience that, once you’re past a comfortable wage, it gets really hard to justify misery with higher wages. If I flip my thinking, there’s an interesting thought… Read more »

Peter
Peter
8 years ago
Reply to  Erik Dietrich

“I’d deliberately introduce things I knew they’d spend a long time arguing about so that they’d run out of time and leave the rest of my code alone. ”

How clever you are:)

tiriana
tiriana
7 years ago

“Look at code reviews (…) as opportunities for groups of people to work toward better solutions.” – I love it. I couldn’t agree more.

Thanks for this post.

Erik Dietrich
7 years ago
Reply to  tiriana

Thanks for the feedback — glad you liked!

adam
adam
7 years ago

Nobody wants to be stuck with an interminable code review, but occasionally the code/author really does need that much TLC. As long as reviewers are timely with looking at revisions, and are constructive/illustrative in their feedback, I don’t see what’s wrong with reviews being a solid gate. I’ve been in both types of teams: ones where a missing period on a comment block would hold up the pull request, and ones where it was normal to pull the code unless there were serious grievances to address. Believe it or not, I strongly prefer the former. If we create a grey… Read more »