Sunday, August 23, 2009

How To Kill a Code Review

Today's guest post is from Gary Myers from Igor's Oracle Lab. I was first introduced to Gary via comments left here. I can't find the first one of course...but he has left plenty of them. All well thought out and informative. Most recently he introduced me to the ability to do a bulk bind using %ROWTYPE here.

This is a topic that is all too often ignored, as you all know.


Steven Feuerstein states here that "Everyone knows that code review is a good idea"

The problem is what happens after the review.

Once upon a time there was a piece of code. That code had been in production for a long time, ran pretty slow but not slow enough that it had reached the top of the pile of stuff people complained about.

A change was done to that code for a new enhancement, and in one of those bursts of enthusiasm that sometimes hits a development team, it got subjected to a code review.

The whole structure of that code was ugly, with unnecessary nested cursor loops. The big kicker for performance was that at the end of one of these inner loops was a TRUNCATE TABLE. Because when you are deleting all the rows from a table, every-one *KNOWS* that a truncate is fastest, right ? Of course, as the TRUNCATE is DDL, it meant that all the SQL using that table inside that loop was getting re-parsed each and every time through the loop.

There were other problems in the code too. I believe one was with variables not being re-initialized at various points in the loop, so there was a risk of incorrect results in some unlikely cases.

The verdict of the review was that the code needed a re-write. The problem was, since it was already in production, no-one wanted to admit that there were bugs in it (and the users hadn't spotted any incorrect results). The new code would go into a future patch, but that wouldn't go live for months. However it had been promised for delivery to a test environment. A rewrite would mean missing the drop deadline.

A quick-fix could be done to improve performance. A rewrite could be done and the deadline missed. The quick-fix could be done and still meet the deadline, then a later rewrite to fix the underlying problems (but those problems probably would have been blamed on the quick fix).

A compromise was reached. Since the change to the code didn't actually introduce any new bugs, it would be allowed to go through to test with no changes from the review. And there was a promise to actually rewrite the code.

Of course, once the delivery was done, lots of other priorities came ahead. I don't know whether the code ever got the rewrite, but I suspect not. Definitely, for at least six months, there was a batch job taking hours when a five minute code change could have cut it to minutes.

At least the developers who participated in the review learnt that a TRUNCATE has drawbacks. The code reviews pretty much never happened again though.

7 comments:

Anonymous said...

Code reviews are critical to software development. It might seem antithetical, but to me, the main purpose of a code review is *not* to find bugs. Sure finding bugs is nice, but there are much greater benefits.

First, holding regular code reviews changes the way developers write code. If they know that someone will be reviewing //every// line of their code, they will be less apt to become code slingers.

Secondly, code reviews establish common programming techniques and styles across a team. This is important in that fewer bugs are created when people do things the same way. Not only are there fewer bugs, but bugs that do arise are easily identified by other team members who might have experienced similar issues.

Thirdly, it's a great tool to bring esoteric information to the team. There are a hundred thousand (probably not an exaggeration) little things that an experienced developer ought to know. Junior developers, no matter how talented they are, just haven't had the time to gather all of those "factlets". Code reviews are an excellent forum to introduce these bits of knowledge.

It is sad that that was the last code review. There should never be anything to hide in development. A team should be motivated to share their work with others. "Look what I can do!" not "Mind your own business!" If this attitude creeps into your organization, you'd better work to fix it, or at least go somewhere else.

-BK

oraclenerd said...

@anonymous

I totally agree. I haven't been able to put into words point #3 so thank you for that.

Point #1 also applies to writing maintainable code, as you don't want to be (playfully) ridiculed for having sloppy code.

Jake said...

AKA Phasing, the mythical phase x you promise to do once whatever milestone has been achieved.

Agile methods like peer programming and refactoring help with this, but once code is in production, good luck getting a refactor done.

The future is good enough.

Jesse Gibbs said...

Atlassian's dev teams do quite a few code reviews (after all, we *do* sell Crucible, a code review tool ;)). We've found it's most effective to review diff-based changes as they are made, rather than trying to review legacy code that has been proven to work in production environments. There are a lot of benefits to this approach. You save bug-fixing time by finding defects earlier, and the developers involved don't need to do a bunch of preparation, because the amount of code being reviewed (typically a single changeset) is not huge.

oraclenerd said...

@jesse

Isn't FishEye an Atlassian product too? Haven't used Crucible yet.

I like that methodology though, just the changeset, for exactly the reasons you mention. What about new products though, or additional features that require wholly different code?

Not that it probably matters in your environment as it sounds like you've got things humming along. Color me jealous. ;)

oraclenerd said...

@jake

Yeah, I run up against that wall all too often.

I haven't learned yet how to fight it (or even if it should be).

What if you have 10 developers maintaining something that should normally take 3? Even if it "works" wouldn't you suggest, at some point, a rewrite?

I've seen an application running on a 3 node RAC Cluster that could just as easily run off...an iPhone. How do you make that sale (time/effort) against the desire to keep purchasing hardware and licenses?

Jake said...

@chet: In my experience, even if you're overstaffing to support old production code, the fear of downtime and issues associated with a refactor is not enough to keep it as is.

Even a cost-benefit analysis doesn't always work b/c loss of a production system inevitably has unexpected ripples.