[coding] How to Code Review People You Like

People who visibly save thousands of dollars are rewarded better than those to silently prevent the loss of millions

--me

I originally wrote this as an answer to a question on a reddit thread, and then realize that the poster (or "OP" in reddit lingo) probably has no interest in reading it.  So, I'll leave it for you guys!


In a software company, a Code Review is a development practice that can make one or both participants look good, or bad, and can be used to slow down a check in.  Sometimes people also find programming flaws.



Your true motivations in a code review can very widely.  Maybe you need to subtly delay a co-worker's check in so that you don't have to deal with an inevitably ugly merge.  Maybe you need to make a show about due diligence while  stealthily introducing a "feature" that is going to make another team's on-call really unhappy in about 12 hours.  Maybe you are code reviewing a friend and you want to make him look good.  Maybe you are code reviewing an enemy and need to make him look bad.  Maybe you are code reviewing an enemy that thinks you guys are friends, and you need to make him look bad without him realizing it.  Maybe you need to code review an idiot, but in such a way as to cause him to accidentally learn something without bruising his ego.  Maybe you need to bait someone into giving you a positive or negative story for peer review time.  Maybe you want your team to lose face.  Maybe you are taking salvos in a religious war about code style, or a religious war about SOA frameworks, or a religious war over configuration, or a religious war with some dumbass principle in another org, or a religious war about some dumbass thing a guy wrote before being promoted off to VP where he can't do as much damage.  Maybe you have your own agenda you are trying to push, because if everyone stops doing this 1 stupid thing, you can decrease your servers in production by about 80% (remember:  people who visibly save thousands of dollars are rewarded better than those to silently prevent the loss of millions).  Maybe you need to make sure there are no bugs or other flaws in the system because you are on call next week.


I'm going to assume that you like these guys, and you have no interest in harming their careers, or making them or their team look bad.  I'll also assume that you have no styling agendas you need to push, and there is no bad blood between your teams.  If any of those assumptions are wrong, stop reading, because you would need a completely different strategy.

If that's all true, then you're job closely aligns with the original intent of the CR:  to improve code quality.  That's the benefit of CRs.  The cost is time.  CRs are not magic, and they are not free.

Every good suggestion you make saves time and money (especially if you prevent bugs going to production).  Every dumb suggestion you make wastes time--time spent arguing with you, time spent recoding, time spent retesting, time spent on CR #2.  It can also cost you social or political standing.  Since you have no interest in embarrassing these guys, or slowing down their checkin, you want to be careful not to make any dumb suggestions.

Based on the tone of your question, it sounds like you are unfamiliar with any standard style guidelines at your company.  You should find out if there are style guidelines for the project.  If there aren't, DO NOT make style suggestions (I'm assuming you dont want them to hate you), except in cases of extreme stupidity.  If there are, you might get away with selectively nitpicking the ones you care about.  If you have your own personal style religion, advance it at your own risk.

Next is the design.  In general, Java devs usually have a hard on for over-designing things that will never change, while ignoring things that are about to.  Interns that have just taken a "software design" course will probably be even worse.  Keep a look out for any major flaws, however restrict your comments to things that are both obvious and easy to explain.

Next, look for dumb mistakes.  The tone of your comment should convey a belief that these guys are smart and it was a slip of the keyboard, even if they are idiots (especially if they are idiots).

Next, look for things that are unclear.  If they do anything weird, they need to add a comment, and that comment needs to answer the question WHY.  Most idiot programmers add comments that answer the questions WHAT or HOW, and then get all pissy about being forced to write comments.  However if its a small check in, and they dont need to change any code, and you're not trying to slow them down, it is probably not worth it to ask them to write comments--unless you also tell them that you don't need another CR iteration before they check in (by the way, the 'I don't need to look at it again' thing is how we...nevermind).

You need to review your own comments.  If this is more than a few lines of code, and it is a public CR (i.e. the whole team, or your boss, or the whole org is going to see) you need to make sure you look like you've given a good review--most of the HR "career management" bullshit about what a great developer is at places like Amazon/Netflix/Microsoft/whatever is tied to people (all people, including idiots) thinking you give good CRs.  Go through the code again, and make sure you nitpick according to the beliefs of anyone important that might see your review.  Are we supposed to hate ThreadLocals?  Bust on them for using thread locals.  Are we supposed to love ThreadLocals?  Find a volatile variable and and ask them it should really be a thread local.  Does the Principle dev hate ThreadLocals while your own manager loves them?   Do whatever it takes to make sure that all of the people with political power think that you have the same beliefs they do.

Also, with most corrections, if you are not blindly reciting things, it is always better to ask, because you still look like you believe all the right things, while they are still held accountable for their answer.  If anything goes wrong, you can say you had to "disagree and commit" or whatever the appropriate corporate lingo is.

You know, this makes me wonder if you could make a sort of "House of Cards" with programmers.

For more information on code reviews, programming style, and working for a large corporation, please see this guide on software development career management.