Stories about Software


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

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.

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.

  • Jake Brown

    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 effective, because someone is always squinting, and because someone is sitting at the keyboard, there’s a much stronger incentive for them to control the entire conversation. You’re better off having all parties at a keyboard, with a tool that lets anyone take control at any point to highlight a question. meeting rooms stink, because they make people fall asleep (even if they look awake).

    ….other than that – be respectful, don’t get caught up in minutia.

    another thing we did is we did have a ‘pass’ review step – but it was just that code had to be reviewed (could be post-check-in, but not post-sprint). but the pass was that both the reviewer and reviewee had to agree, and for the most part anybody was game for a review – ie, it was not gated by the ‘SENIOR’ developer. and the criteria for passing was, ‘this code is reasonable enough that we can continue to improve it, even if it may have some current flaws.’

    i also like review-per-check-in rather than review-per-feature, because usually by the time you finish a feature you don’t have the willpower or time to go back and make major changes if someone else has a great suggestion.

    ….with that kind of review, i had the most positive mentoring experience in my career. they grew a lot during a time, and so did I.

    ….i did work parallel other teams that found this strategy to be abhorrent. they felt this opened the gates for ‘sub par code’. they liked to do ‘bug detection’ code reviews, and fine-toothed combing. in the end, they had a silo of a couple productive members, and a bunch of smart people who were jaded and demoralized, and unproductive.

  • http://www.schmonz.com/ Amitai Schlair

    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.

  • http://www.christian-fey.com Christian Fey

    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 area of code you touch rather than only one. As long as the culture expects this type of buy-in, defensiveness disappears, and more often than not, those other reviewers only review their own section of source files.

  • Jake Brown

    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.

  • Richard Buckle

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

  • Robby

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

  • Not Ben’s Fan

    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

    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.

  • Pingback: http://www.daedtech.com/how-to-use-a-code-review-to-execute-someones-soul | laudbrian()

  • BengtQ

    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.

  • A

    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 stuff, right? Oh, they’ve added a comment for themselves on a bit they wrote, by f- that’s a violent self-criticism. Still even if their tone with me is politer than with themselves it’s a non-issue.

    Senior dev keeps saying things as if they’re fact but I know better, rather than look into it and maybe learn something I’ll simply point out that any experience they’ve had prior to my arrival is doesn’t matter, so what if they claim that there was a time when they said they’d never use regex either, I’ll never use it.

    I wish the senior dev would point out when i’ve done my job more often, I feel that while 1 loop deserves an accolade. I bet they’ll be me appreciative if I undermine them whenever a chance arises. Perhaps the senior should spend less time trying to push me towards learning something without phrasing it as such and more time talking about me.

    And don’t get me started on not being able to push to production, after two months it’s as if I practically built this company.

    Rather than defend the gatekeeper or crazy notion of minimum code quality, I’m trying to show that the expectations of the junior and the interests of the employer don’t necessarily align. Ignorance of internal politics

    The senior isn’t trying to get you to question whether or not you are good enough at life to work, it’s getting you to ask whether or not you’re understanding of your field is sufficient to hold your position. Sure you’ll hate them but instead of complaining why not show a little humility and consider the possibility that you’re not the three-month-legend you once thought.

  • http://www.daedtech.com/blog Erik Dietrich

    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 for someone else — just counter-productively cathartic for me, when I slip and do it.

  • http://www.daedtech.com/blog Erik Dietrich

    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).

  • http://www.daedtech.com/blog Erik Dietrich

    Thanks — will have to take a look!

  • http://www.daedtech.com/blog Erik Dietrich

    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.

  • http://www.daedtech.com/blog Erik Dietrich

    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, the dynamic I’m describing is one where people use positions of seniority as excuses to be nasty and get away with it. The dynamic that you’re describing is where someone is incompetent at his job. The standard performance review/PIP system is theoretically designed to handle the latter, but not the former. I don’t think that responding to a non-performing team member by being nasty is likely to have a good outcome. If junior dev is checking in bad code, being nasty and refusing to learn from mistakes, being hostile and combative may be satisfying, but it’s not really helping anything. With someone like you’re describing, a good feedback system like the one accompanying Scrum is sufficient in and of itself to make it objectively clear that it’s an “addition by subtraction” situation.

    I sympathize/empathize with your point. I can think of no faster way for a junior developer that I’m managing to fall into my bad graces than to be arrogant and unwilling to learn from mistakes.

  • http://www.daedtech.com/blog Erik Dietrich

    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.

  • A

    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 exhausted “c’mon, trust me, I don’t want to tell you how to do it but there’s plenty of opportunities to improve this code, you’ve been coding the language for a while now, see how you’ve progressed” which I guess highlights the potential for situations where arrogant senior and incompetent junior are imaginary, born of misinterpretation by both parties. If that’s the case then perhaps they could be mitigated by indicating early on that the often terse senior believes they got a rough idea of how to meet company and junior’s expectations and doesn’t appreciate being challenged on every decision. Or that the junior sees a rewrite without direction as the set up to a character assassination and not having at least the ability to modify bespoke components as an insult.

    Admittedly there are some personality aspects that are always going to be detestable and a grand solution isn’t likely on the horizon. Being comparable to the blog character I’d offer this advice to juniors in this situation: Run with it for a while, jump a few hoops and gamble big falls; if it doesn’t change ask yourself how much effort you’ve put into improving, if the answer to that is “I spitefully read the book on coding the last few commits”, then it’s the senior and worth getting out. Just remember you might one day be on the other side of the fence.

  • Pingback: The Baeldung Weekly Review 20()

  • dave falkner

    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.

  • Chris Landry

    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 thing should be nitpicked, and if I need to provide feedback on style I tell people that their code is wonderful because I’m able to point out the nitty gritty stuff instead of big issues with logic or execution.

    You are totally correct though that this type of feedback is best provided by an automated tool once one is in place.

  • Pingback: 代码审查是如何抹杀开发者积极性的? | | Evolution Unit 进化Evolution Unit 进化()

  • Pingback: Armchair Tour Guides and Presenters | DaedTech()

Acknowledgements | Contact | About | Social Media