DaedTech

Stories about Software

By

Avoiding The Politics of Code Review

Editorial Note: I originally wrote this post for the SmartBear blog.  You can check out the original here, at their site.  They have a number of authors over there that are writing good posts.

Asking someone to look over your work is a pretty common practice in any discipline. We’ve been conditioned with the idea that it’s always good to have a second set of eyes look at your work prior to officially marking it as done.

Writing and reviewing code is no exception.  It’s pretty easy to forget to check an argument for null, so having someone review your work only makes sense.  In fact, it makes so much sense that software departments tend to formalize and mandate the process, calling it “code review.”  And the practice is quite effective.

But formalizing and mandating a process in a corporate setting can lead to a variety of unintended consequences.

NuclearDuck

Office Politics of Code Review

I often quip that if an endeavor has more than two people, then it also has politics.  Furthermore, if you think that it doesn’t, you’re likely starting off at a disadvantage because you’re playing a game without realizing that you’re playing.  In the office setting, where money, power, and influence are at stake, politics tend to be magnified.

If your department has code reviews, then your department has politics around them.  The ideal state is that these politics are minimized to the point of being unnoticeable, but that is often not the case.  In fact, they’re often so noticeable that they’re toxic.  Is that the case with your department?

Let’s look at some types of personalities you may have to deal with in the office and ways you can avoid office politics anti-patterns around code review.

“The Big Man on Campus”

The “big man on campus” (BMOC) is probably the least intricate political situation imaginable, though it has the potential to be among the most counterproductive.  In this situation, there’s a pushy personality that cows other group members into coding according to his will.  Usually this personality tendency is reinforced by organizational seniority (and not necessarily competence), with the BMOC having a title like “tech lead,” “architect,” or, at least, “senior software engineer.”  However, that need not be the case; the BMOC can simply be a particularly aggressive and domineering team member.

You can recognize this political situation and the crux of the problems that it causes by the words and attitudes of other team members.  “Oh, Bob’s not going to like that. You’d better take it out.”  The best case scenario is that the BMOC is actually quite sharp, technically.  When that’s true, he’s just a bottleneck — a micromanager that inspires learned helplessness in the team.  The end product will be good, but the effort won’t scale well.  The worst case scenario is that the BMOC is technically incompetent and rules through pushiness alone.  When that’s the case, you still have the learned helplessness and the scale problem, but the work product is also poor.

“The Cool Kids”

If the group with a big man on campus is a monopoly, the cool kids anti-pattern is an oligopolyThe software group is not ruled by a single dictator but by a cartel of established, senior people.  The cool kids don’t necessarily agree on everything, but they agree on a lot of things, and they tend to keep their infrequent disagreements in-house.  The reason disagreements are seldom and private is that retaining their status as members of the in-crowd is more important than being right and more important than the software.  A cool kid might go along with something like global variables not because she thinks that they’re a good idea but because the other cool kids might gang up on her and excommunicate her if she goes against them on the point too often.

This anti-pattern is most recognizable by new developers always, somehow, being wrong about everything.  A newly-hired developer is often a goldmine of fresh perspective, but the mine will remain untouched if the cool kids rule the roost.  Admittance to the inner circle is hard-won and requires a lot of invested time and proved loyalty.  In the meantime, good ideas and practices are discarded if they come from the wrong people.  The danger with this anti-pattern is that it promotes coding monoculture and limits ideas to the most long-tenured and thus the most insulated people on the team.

Tournament Ladder

If the cool kids split the group into the insiders and outsiders, the tournament ladder dispenses with the ability for people to rest on their laurels.  Like its namesake sports structure, the person on top can be displaced at any time, and all competitors can move up or down.  The most important thing here is being right about any detail, however obscure or trivial.  You can move up the ladder by proving that your deserialization scheme shaves five microseconds off the existing one.  You can move down the ladder by being wrong about some arcane piece of compiler trivia.

You’ll know you’ve wandered into a tournament when every code review seems to give rise to competitive sharpshooting of anyone’s claims and constant, obsessive research to prove one’s correctness.  People will spend hours preparing their defense of their code ahead of review.  At first blush, this seems like the sort of healthy competition that drives everyone toward improvement, but that turns out not to the case.  In the first place, it makes everyone neurotic and is hard on morale.  And, even more importantly, the focus isn’t on delivering value through software or even getting the software right because the software is really just the tournament venue.  Rising on the ladder and defending against upstarts is all that matters.

Avoiding it All

These are far from the only political arrangements that might crop up in your group if you aren’t careful, but they’re the most common ones that I’ve seen in my travels.  What you’ll notice is that, while the game changes in each scenario, the fact that the game trumps the well-being of the software is shared by all scenarios.  The more politics there are around writing and reviewing the code, the less productive the writing and reviewing will tend to be.  You need to minimize it.  Here are some ways to do that.

  • Ensure that reviews are two-way. Never have people who only review and people who only get reviewed.
  • Always focus on the code and not the person who wrote the code.
  • Make the reviews small, frequent, and informal. Marathon group sessions in rooms make people defensive.
  • Frame things as questions and suggestions rather than orders and accusations. Ask that others do the same.
  • Automate as many checks as possible so that reviews don’t focus on simple details.

But, in my experience, the single most important thing you can do to cut down on politics is to make taking the feedback as optional.  You don’t “pass” or “fail” code reviews, and you don’t get your code “approved.”  Instead, you ask feedback and take it when you find it valuable.  That preservation of autonomy is absolutely essential to having a good, collaborative environment.

Removing the gatekeeper nature of a code review may seem risky, like leaving your house unlocked.  And, I’ll concede that a stubborn developer leaving a defect in the code is a risk.  But it’s nowhere near the level of risk that you run when your code reviews are more about politics than about improvement.  That is a risk that you cannot afford.

  • Mike Parker

    Great article. You remind me how many “much worse” places there are to work 🙂

    Also, this quote: “the single most important thing you can do to cut down on politics is to make taking the feedback as optional.”

    I definitely agree, however there is a small exception for junior / graduate developers who have an extreme amount to learn. Often code reviews early on are bringing them up to speed with your culture, code style, practices etc as they push the boundaries and learn “the basics”. Ideally you want people to move towards autonomy as fast as possible, but you can’t hope to treat a complete junior the same way as an expert senior. Different strokes for different folks, as they say.

    • Thanks for the kind words!

      In my experience, new hire juniors tend to be terrified of breaking things. I’ve more frequently had to coax them into delivering than try to stop them from doing so.

      I think, philosophically, I wouldn’t introduce approval steps, except maybe as a consequence of poor decision making. So, I’d let the juniors take or leave the feedback, but perhaps have a policy in place of “if you introduce problems as a result of feedback not taken, the feedback will become mandatory.” (This is sort of off the cuff, just meant to convey the idea). I hate to take away autonomy, except as a last resort. But if loss of autonomy is a consequence of poor decision making, autonomy is still preserved after a fashion.

  • Michael Harrington

    Nice article. I think that a very useful approach is to try and have a large amount of common ground and automate it if possible. Use as many of the Code Analysis/Stylecop type tools as appears reasonable and proportionate to the project and are supported for your technology stack.

    The advantage of doing this is that it removes most of the most frustrating types of code feedback from the process by providing an objective standard. A secondary advantage is that with the lower-level code problems off the table, a code review can focus on the design of modules, heuristics and algorithms rather than debating syntactic elements.

    Another useful thing I’ve seen used is to informally time-box the code review to stop the effort getting out of hand or disproportionate. I would be very concerned if I were leading a team and I found code reviews taking a long time, or people preparing detailed defences. It is a cross-check to respectfully refine the work of another who is considered skilled and competent, not a gate to eliminate the inevitable errors of someone untrustworthy.

    Finally, it is quite nice to insist that a code review is a condition of checking code in, even if it is optional to act on the feedback from the review.

    • I’m definitely a huge advocate for automating as many checks as possible. I like how it tightens up the feedback loop and lets people learn and course-correct on their own.

      Your other suggestions make a lot of sense to me as well.

  • Great article. Really liked it.

  • David S

    Again, thank you for this one! Several months ago, the team I was on implemented a code review policy which embodied several of these pathologies. Since them I’ve been wrestling with it in my head, trying to understand how such a promising, universally lauded practice as code review could go so badly wrong. This reassures me that I wasn’t crazy. I think I eventually even came to similar conclusion, that just making the feedback into suggestions rather than orders would clear away 80% of the power politics

    • Glad you liked. It’s surprising to me how frequently the dynamic you’re mentioning occurs. It seems like, “hey, get a second set of eyes on this” would be so simple and benign, but it can easily play out in ugly fashion.