Code Reviews

One of my month-end deliverables this month was to have finished reviewing certain portions of code that my team has written.  I have spent the last two hours going throug 20 printed pages of code. (I print the code out because I find it a lot easier to reference multiple sections of code at once using paper than I do using a monitor)

I feel like I’m back at varsity marking pracs and tuts (which I did for 3 years) or like I’m back doing my 1 year of part time Computer Studies teaching and I’m marking the Grade 12 tests.  The 20 pages I have looked at are almost more covered in ink from my pen than in the pigments from the laser printer.  (ok, so that’s a slight exaggeration)

The code I have to review is probably 30% mine and 65% developer A, and 5% developer B.  All of us are pretty pedantic, but I’m quickly seeing trends.  I think I can now make out Developer A’s code at a distance, wearing a blindfold, with both my hands tied behind my back.  I don’t think I’ve seen any of Developer B’s code yet, but we have recently been through a rather large “refactoring” of our OO heirarchy so I think there are bits of his code which I’ve looked over tonight. And I’m still amazed at how my own code sucks.

A lot of our development has been evolving as we’ve gone along so there are some cases of really old and BAD code, right along side really new and slick code.  Then there are cases of really old and BAD code being copied and pasted into really slick and new code without even changing the comments to match their new location!

There are cases where one developer built one way of getting access to a list, and then a few months later built another way of getting access to the same list and put it right next to the first way in the code and didn’t pick up on it.  I’m not sure if I am that aware of the code that surrounds my current coding task, but I’m sure as heck going to start trying to be in future.

One of my pet peeves has been comments in the “///” comment portions ahead of method/field/property/class/etc declarations which are written assuming a fair amount of knowledge of how our system works, instead of written to explain to a new developer how the system works (I’m guilty of this too).  One of the things we’ve discussed many times is the fact that eventually we want to produce NDoc/DoxyGen documentation from our code so having comments that are not helpful is really a waste of time in the long run.  (Worse, in my mind is repeatedly starting sentences with lower case letters – something that is uber evil when you consider how many members of the the grammar police are in our team)

Fortunately, after our recent OO Architecture “Refactoring” Marathon, most of the code I am reviewing is consolidated into three main base classes, and I’ve completed the review of two of the three.  So after the next class I should have very little code left to review.

I’m still rather suprised about the amount of silly bugs we have in our code… I’ve been watching Scrub’s recently and I keep saying to myself (in the voice of Dr Cox) “Oh, my goodness barbie!  How do you get your bra and panties on in the morning all by yourself!”

I guess that’s why we have code reviews.  We try to have “debriefing” (after my “bra and panties” comment, I must say – “No, not that kind of de-briefing!”) sessions where we all try and learn from the mistakes made in code that is reviewed.  So I have LOTS of content for our next session, and hopefully I won’t see these mistakes ever again.  (I can dream, can’t I?)

 

So after that general badmouthing about the quality of our code, I must just say that we have a great team.  If(when) we make mistakes we learn, and code better.  The other guys are super bright and really capable (which is probably why some of th bugs shock me so much) and I’m really happy to be working with them.

Leave a Reply

Your email address will not be published. Required fields are marked *