Archive for September, 2008
Code Comments are not for tracking changes
Recently, I have been reviewing and correcting a lot of broken code, and in doing so, I have concluded that the number of developers who know how to properly use a bug tracker and version control is not nearly high enough (disclaimer – this is in a big faceless corporation).
Commenting Anti-Pattern
So what leads me to this bold conclusion? Have you ever seen code that looks like this:
begin -- start change for #3245 v_myvar varchar2(100) -- end change for #3245 for r in (select c1, /* start change for #9870 */ c2, c3 /* end change for #9870 */ from foo_table where c1 in (1, 4, 6) -- commented out for #9870 -- and c2 = 'val1' -- start change for #5678 and c3 = 'val2' -- end change for #5678 ) loop null; end loop; end;
Ignore the fact that this code does nothing useful, and is PLSQL – its just there to illustrate a point.
The amount of code I have seen recently that has its changes tracked through inline comments that attempt to encapsulate every single line changed to fix a bug or add a feature is unbelievable.
Some may ask, what is the big deal? Well firstly, when I see code like this, it says to me, the developer who did this, doesn’t have any idea about version control – they are commenting every change to attempt to provide traceability and rollback ability to their change. Often, when I see this anti-pattern, I subsequently find the code hasn’t seen the inside of Subversion or anything like it.
Show me the code
If I ever dare to ask to see the changes that were made to fix a bug in code like this, I get handed a source code file and told to grep through it for the bug number – what scares me, is how do I know he remember to comment every single change? For a big change, it takes a lot of patience to do something like that! And then the Delivery Manager (DM) arrives at the developers desk and the conversation goes like this:
DM: Remember those 10 feature requests we agreed to deliver tomorrow, well how are they going?
Dev: No problems, we have them all finished and tested, ready to roll!
DM: Well, we have a problem – turns out the front end developers have not been able to complete feature #4567 and that impacts us – we need to deliver the other 9 changes, but not that one.
Dev: Huum, thats going to be a problem, #4567 was a big change – I don’t think we can get it out of there without a lot of effort!
DM: Don’t you have all this tracked in your version control system … back in my day we committed individual changes into SCCS and things like this were never a problem!
Dev: Gulp … guess its going to be a long night for me then sir …
Finally, don’t even get me started on how much harder the code is to read with all these annoying ‘starting/ending’ change comments.
A simple recipe
If you come across this rant, and have found yourself doing this, then stop a moment to think about how to use version control to track these changes for you – thats what is was designed for!
Here is a tip – when you have to fix a bug or add a feature to existing code:
1. Checkout your code
2. Make changes to whatever files are necessary and forget about pointless start/end change comments.
3. Get your test suite to pass your bug/feature (you have a test suite, right?)
4. Check in you code when its all done, quoting the bug number in the commit comment
Simple, clean code changes, traceable in a much more robust way and you can safely roll back the changes to any bug at any time.
The biggest mistake I see developers making with version control, aside from not using it at all, is to fix about 10 bugs, and then do a commit – please don’t do this, as it removes the ability to examime the changes made to fix a single bug, not to mention removing the changes if the need arises – which happens more than you may think, as our poor Dev found out above!