Aaargh, Part Six: One More Thing About Comments

Aaargh, Part Six: One More Thing About Comments

Rate This
  • Comments 16

Gripe #7: Use The Right Struct For The Job

I meant to include this one in yesterday's gripe about comments, as this illustrates a time when I found a comment that should never have been there.  The person who wrote the comment should have realized that the very fact that they needed to put a comment in indicated that the code needed to be rewritten.  It slipped my mind at the time though, so here it is today.

Here's a fragment of code I found in an unnamed Microsoft product once. The code is part of a fairly complicated routine for clipping a rectangle -- the details are unimportant for the purposes of this discussion. The code is correct -- it performs the correct calculation and spits out the right answer -- but boy, is it hard to read:

if (client.x > rect.right)
  default.x += ((client.x - rect.right) / 2 - rect.left);
// right == width
if (client.y > rect.bottom)
  default.y += ((client.y - rect.bottom) / 2 - rect.top);
// bottom == height

Holy cow, was that ever a bug waiting to happen. The data member that holds the width was named right and the data member that holds the height was named bottom ! I needed to change this code and spent a good thirty solid minutes looking at the routine (which was much longer than shown here), running it in my head before I concluded that the comment and the code were in fact correct.

Someone got lazy -- they already had a struct defined for a {right, left, bottom, top} rectangle but they had the information {left, width, top, height}, so they just stuffed the information they were manipulating into the struct they had handy. Aaargh! It's drivin' me nuts!

Worse, the comment is cryptic in the extreme.  It took me quite a while to figure out whether the equality operator in the comment was documenting a logical invariant (that is, the comment means “we know that the right coordinate is equal to the width because the left coordinate is always zero in this case“) or whether they were just using the equality operator to mean “has the semantics of“. 

Please don't put cryptic comments in the code explaining why code is misleading -- fix the code so that it isn't misleading anymore! In this particular case, the rectangle in question was entirely local so there was no need to have it in a struct at all. I needed to modify this code anyway, so I got rid of the struct altogether and just declared four local variables for the left, width, top and height, appropriately named.

  • I know nobody will ever read my comment, since the article is from 2004, and it's 2010 (we're getting old, uh?). I heard somewhere once (probably one of my teachers back in college) that great code is written for humans, poor code is written for the compiler. And that we should always do the previous.

    In this case, the compiler doesn't care whether the variable is called top, bottom, right, left or sky, floor, orient, occident. It only cares that it's a integer. But for humans, it makes a very large difference.

Page 2 of 2 (16 items) 12