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.