Code Reviews Are For Critics

OK, so this is coming from a need to redeem myself. I screwed up and the only way that this will stop making me so miserable is if I help another developer not make the same mistake and then have to suffer the consequences such as being jolted out of sleep at 5 AM on a Saturday by a nightmare where you are being surrounded by Int32s overpowering you with the help of the Convert method and trying to change the Real you in to one of them. Its all over in a fraction of a second and you are now an Int32, you've either lost something that was yours or you've acquired some of their characteristics, you'll never be the same, you have been assimilated, you are now... well, them!!.


The Cast in My Dream


So this is how I screwed up.

I wrote
BillAmount = convert.ToInt32(row["bill_amount"]);
instead of
BillAmount = convert.ToDouble(row["bill_amount"]);
when hydrating an object with the data coming back from a stored procedure.
(Writing those lines again just gave me the shivers and I'm a little queasy.)

The erroneous cast of course rounded the cents in the amount so if the amount was $4.95 it now ended up being $5.00 or if it was $4.45 it would now be $4.00.

The Bug In the Machine

So now that we know how I screwed up...the question now is why didn't I see this before I even committed the code? This could have been caught before we went to production with this... ya we did .. we found the damn bug scurrying around in our production machines and killed it there before it turned dark green, grew fangs and a pointy tail and ate the pilot.

So the world is now safe from that monster ... but why did we miss it in the first place? we had the processes in place to catch these things before they go out to production, we wrote the unit tests, documented the project, did the code review, went through QA, passed the Certification Tests, and finally deployed to production after a pretty intense and rigorous process.


Tracking The Bug

I think the answer lies in how we finally found the bug. As the saying goes, the devil is in the details, sadly not all details are of immediate concern to everyone on the project, and among the myriad of things that can go wrong in a software project it is easy to miss several important but minor details.

An easy way In my opinion to get these details straight could be to have a check list that should be gone through at every stage in the life cycle of the project. I know, we don't need another document, and another process to follow, however if we think about it all this Check list is, is the requirements in a condensed format.

The check list should just be a set of expectations/issues that the software project has to resolve. This check list should be written by each stake holder in a project at the beginning of the project and updated as/if expectations/requirements change during the course of the project. Some or most of these could even be translated in to unit tests that would ensure that the goals are being met.

This check list should be used by each stake holder at various stages in the project's life cycle to then verify that his/her expectations are being met and if not then require that the cause be investigated until a satisfactory answer is received. That is to say If Ya See Something SAY Something! (That is essentially how we found the bug! Someone saw the problem and asked the question.)

The point of the checklist is to keep track of the details and to have a mechanism so they are tracked. This check list could be something that each stake holder goes through at the end of a design review, code review, QA test plan and product Demo. I don't think it should be the only focus of the stake holder as this will tend to limit the scope of say a design review or code review to just those set of concerns and could cause other issues to be missed.

The sign off to production should include the items on each stake holder's check list being checked off or crossed off. No item should be left unaddressed.

Critics Rule

It is imperative for a software team to have the resources and the process in place to identify and fix bugs. However this wouldn't mean much without a critical approach.

This is exactly the reason why a developer must not QA his/her own code, the point is that someone other than the developer should take a more critical view of the code and thereby help prevent bugs in the system.

The normal process is to have meetings to review the code that has changed and then sign off on the code once it is found that the accepted coding practices have been followed and that nothing has been overlooked. This is meaningful only if the team in the room is paying attention, is knowledgeable and critical.

Alternatively it could be useful for one to hand over the code to another developer to review, run unit tests, evaluate aspects of the code that are not easily done by automated build tools such as test the logic and recommend a better way of doing something, read the documentation in the code to ensure it is understandable, etc.

Regardless of how this is done it is imperative that code is read and critiqued before it moves out of dev.