The Fine Art of Being an Asshole in All the Right Ways

We do code reviews of all code that’s about to be checked in. It’s interesting to see the different approaches to code reviews different coders have. Many (especially junior) programmers tend to lack the self confidence required to go about a code review in an effective manner.

Let me explain that — why would a code review require self confidence? Well, a code review will go over a large amount of code at times, and your job as the reviewer is to find any question marks that might possibly cause any kind of trouble. This is, apparently, a problem for many programmers when they review the code of another (usually more senior) programmer, since pointing out potential errors or problems is seen as somewhat sensitive. I know many coders get very emotionally attached to their code.

You would think this should be something which happens only to junior programmers, but I’ve seen this happen with relatively senior programmers as well. It’s more about character than it is about experience I guess. The reasoning is “I don’t understand this, but this guy’s smart so it’s fine” or something similar, sometimes connected to how much code there is to go through and how long that would take.

However, the quality of a code review is directly proportional to how much you can disconnect your own respect for others. Your task is to understand everything you get shown, and to be the most nitpicking, hard-to-please obsessive-compulsive asshole the earth has ever seen for as long as the code review lasts.

Sound like something you don’t want to do? I hope so. But remember, you’re doing a service here… if there are bugs, someone is going to find them. We have QA people who specialize in finding bugs and piling them up on your desk, producers with a great attention to detail, and if nothing else you can bet someone will find it and it’ll end up on youtube. I don’t know about you, but at least I’d want to see my errors pointed out to me before I check them in.

So here are my tips for doing a code review:

• Understand everything. If you don’t understand something, make the reviewee explain it again in a different way. Chances are, you aren’t understanding because there’s something strange going on, or because what’s been done is unnecessarily complicated. Either way, if you don’t understand something you can’t guarantee that it’s right. Part of the purpose of a code review is for the reviewee to explain his thinking, and while doing so make for a possibility that he or she discovers errors as well.
• Be picky. Point out all details, from naming convention errors to coding standard misses to comment spelling errors. Whenever something gets checked in, the cost of changing it is a lot higher. Often the details may be the things that are wrong, and being picky forces you to notice such things.
• Look at everything. Resist the temptation to skip files where only small things have changed, but like I mentioned above, the details can often be what’s wrong. I can’t begin to count the amount of code reviews I’ve done where we’ve found inverted if conditions and equally dangerous things. The relevant code is simply a missing exclamation mark, but the consequences are far reaching, and if you had skipped the file because it had almost no changes in it, then you would have missed it.
• Endure. If you feel like you’re being a pain in the ass, think of it as a great favor since the amount of potential bugs you’re stopping is high. It’ll also act as an incentive for the reviewee to focus more on code quality for next time.

Think of yourself as the guardian of quality, personally responsible for the quality of the code you’re reviewing. Imagine that you’ll be the one yelled at if the code is broken, or the one who’ll be put to fix it.

The end result will be much higher quality code.

3 Comments

• 

  By Peter Jaric, Friday, March 13, 2009 @ 15:11

  My experience is that I often find stuff that are not actual errors, but design issues. These will not cause bugs in the short run and can not easily be faulted that way.

  I am talking about code that should be abstracted, or code that could be changed to use a common design pattern, etc. It is much harder to persuade the coder to change that kind of code than to point out an obviously missing !, unfortunately. I just wish everyone understood that I am the authority on these matters .

  And about logic errors, I’ve also noted that unit tests often find them before the code gets to the code review stage, but someone has to review the test code too, I guess.

  All in all, a very interesting post, though, and I totally agree with you that there’s no point in doing a code review if you don’t point out the faults!

• 

  By slicedlime, Friday, March 13, 2009 @ 15:17

  Interesting comment. We tend to do something more akin to a “design review” — you’ve got to discuss your design ideas with someone before you go implementing them. You’re absolutely right in that after all the code’s done, design errors hurt much more to fix.

• 

  By Gregg Sporar, Monday, March 16, 2009 @ 19:51

  >be the most nitpicking, hard-to-please obsessive-compulsive asshole the earth has ever seen

  Fair enough, but keep in mind that diplomacy counts – in other words, it’s not just what you say during a code review, but also how you say it. More thoughts on that topic here: http://smartbear.com/docs/CodeReviewSocialEffects.pdf

Other Links to this Post

RSS feed for comments on this post. TrackBack URI

Leave a comment