DaedTech

Stories about Software

By

Spring Cleaning Your Code Review

Editorial note: I originally wrote this post for the SubMain blog.  You can check out the original here, at their site.  While you’re there, have a look at the automated code review tool, CodeIt.Right.  

Many of us have a natural tendency to let little things pile up.  This gives rise to the notion of the so-called spring cleaning.  The weather turns warm and going outside becomes reasonable, so we take the opportunity to do some kind of deep cleaning.

Of course, this may not apply to you.  Perhaps you keep your house impeccable at all times, or maybe you simply have a cleaning service.  But I’ll bet that, in some part of your life or another, you put little things off until they become bigger things.  Your cruft may not involve dusty shelves and pockets of house clutter, but it probably exists somewhere.

Maybe it exists in your professional life in some capacity.  Perhaps you have a string of half written blog posts or your inbox has more than a thousand messages.  And, if you examine things honestly, you almost certainly have some item that has been skulking around your to-do list for months.  Somewhere, we all have items that could use some tidying, cognitive or physical.

With that in mind, I’d like to talk about your code review process.  Have you been executing it like clockwork for months or years?  Perhaps it has become too much like clockwork.  Turn a critical eye to it, and you might realize elements of it have become stale or superfluous.  So let’s take a look at how you can apply a spring cleaning to your code review process.

Beware The Cargo Cult

During World War II, the Allies set up a temporary air base on an island in the Pacific Ocean.  The people living on the Island observed the ground controllers waving at inbound planes to help them land.  Supplies then followed.  Not understand the purpose of this ritual or the mechanics of airplanes, the locals learned that making these motions brought planes with supplies.  So after the allies left, they mimicked the behavior, hoping for additional resources.  This execution of ritual without understanding earned the designation “cargo cult.”

In the world of software development, cargo cult programming involves adding code without understanding what it does.  You added it once, good things happened, so now you always add it.  You can think of this as a special case of programming by coincidence.  And it’s something you should avoid.

But cargo cult mentality can crop up in code review as well.  Do you find your team calling out ‘issues’ during review, but, if pressed, nobody could articulate why those are issues?  If so, you have a cargo cult practice, and you should cull it.

Going Over the Same Stuff Repetitively

Let’s say that your team performs code review on a regular basis.  Does this involve an ongoing, constant uplift?  In other words, do you find learning spreads among the team, and you collectively sharpen your game and constantly improve?  Or do you find that the team calls out the same old issues again and again?

If every code review involves noticing a method parameter dereference and saying, “you’ll get an exception if someone passes in null,” then you have stagnation.  Think of this as a team smell.  Why do people keep making the same mistake over and over again?  Why haven’t you somehow operationalized a remedy?  And, couldn’t someone have automated this?

Keep an eye out for this sort of thing.  If you notice it, pause and do some root cause analysis.  Don’t just fix the issue itself — fix it so the issue stops happening.

Inconsistency in Reviews

Another common source of woe arises from inconsistency in the code review process.  Not only does this result in potential issues within the code, but it also threatens to demoralize members of the team.  Imagine attending a review and having someone admonish you to add logging calls to all of your methods.  But then, during the next review, someone gives you a hard time about logging too much.  Enough of that nonsense and team members start updating their resumes rather than their methods.

And inconsistency can mean more than just different review styles from different people (or the same person on different days, varying by mood).  You might find that your team’s behavior and suggestions during review have become out of sync with a formal document like the team’s coding standard.  Whatever the source, inconsistency creates drag for your team.

Take the opportunity of a metaphorical spring cleaning to address this potential pitfall.  Round up the team members and make sure they all have the same philosophies at code review time.  And then, make sure that unified philosophy lines up with anything documented.

Cut Out the Nitpicking

I’ve yet to see an organization where interpersonal code review didn’t become at least a little political.  That makes sense, of course.  In essence, you’re talking about an activity where people get together and offer (hopefully) constructive professional criticism.

Because of the politics, personal code review can degenerate and lead to infighting in numerous ways.  Chief among these, I’ve found, is excessive nitpicking.  If team members perceive the activity as a never ending string of officious criticism, they start to hate coming to work.

On top of that, people can only internalize so many lessons in a sitting.  After a while, they start to tune out or get tired.  So make the takeaways from the code review count.  Even if they haven’t gotten every little thing just so, pick your battles and focus on big things.  And I file this under spring cleaning, since it generally requires a concerted mental adjustment and since it will clear some of the cruft out of your review.

Automate, Automate, Automate

I will conclude by offering what I consider the most important item for any code review spring cleaning.  If the other suggestions in involved metaphorical shelf dusting and shower scrubbing, think of this one as completely cleaning out an entire room that you had loaded with junk.

So much of the time teams spend in code review seems to trend toward picking at nits.  But even when it involves more substantive considerations, many of these considerations could be automatically detected.  The team wastes precious time peering at the code and playing static analyzer.  Stop this!

Spruce up your review process by automating as much of it as humanly possible.  You should constantly ask yourself if the issue you’re discussing could be automatically detected (and fixed).  If you think it could, then do it.  And, as part of your spring cleaning, knock out as many of these as possible.

Save human-centric code review for focus on design considerations, architectural discussions, and big picture issues.  Don’t bog yourself down in cruft.  You’ll all feel a lot cleaner and happier for it, just as you would after any spring cleaning.