Daily WTF
I often read the “Daily WTF” because there’s something satisfying about seeing other people’s bad code. “See? Our code isn’t as bad as all this!” It’s not as fun when you find “Daily WTF” moments in your codebase.
Today, one of my coworkers asked about a piece of code that wasn’t giving the expected results. It’s a part of some really old code that would be done differently given the time to rewrite it. Any way, there are a series of if/elsif/else clauses that check various things. One of those things is to validate some data against a known constraint. This particular section is supposed to validate that the given data falls within the range of a valid UINT (UINT8, UINT16, etc).
elsif ( $constraint =~ /^UINT(\d+)/ )
{
$start = 0;
$end = ( 0x1 << ( $1 - 1 ) );
}
That looks right, doesn’t it? Maybe… If you just glance at it…
Start with 1 and bit shift it left $1 - 1 times. So if we are validating a UINT8, we take 1 and bit shift it left 7 times which gives us… 128. Uh, that’s not right.
It should be written like this:
elsif ( $constraint =~ /^UINT(\d+)/ )
{
$start = 0;
$end = ( 0x1 << $1 ) - 1;
}
Start with 1 and bit shift it left $1 times and subtract 1. So given a UINT8 again, we take a 1 and bit shift it left 8 times and subtract 1 leaving us with 255. Good.
It might be better if we did away with the bit shifts altogether though and did it using a power of 2 like this instead:
elsif ( $constraint =~ /^UINT(\d+)/ )
{
$start = 0;
$end = 2 ** $1 - 1;
}
I haven’t looked at the commit to see who made this mistake because there’s about a 50/50 chance that it was me.
I'm married with 2 girls. By day, I work as a Senior Design Engineer (full time Perl programmer) for EFJohnson Technologies, a Land Mobile Radio company, in the Dallas/Fort Worth area. By night, I play various games as the mood strikes me. (Lately it's CPAN smoke testing.)
:) Btw, is there something special for writing 1 as 0x1?
I don't typically use 0x1 for 1 and that's the reason why it's only 50/50 chance I wrote it instead of 80/20. :)
I like to use the hexadecimal notation just to further announce that I'm thinking about bits and storage.
However, I think I might have written this code less C-like so I could get rid of the conditional:
$start = 0;
$end = $upper_bound{ $constraint };
You might define the hash like you did for your fix, but at least you can hide that somewhere out of the way.
The more that I look at this, the more that I want to look at the commit details (and probably will later today when my curiosity gets the best of me). It looks like something that I *could* have done but it really doesn't look like something I *would* have done. I think I know who would have written code like this *on purpose* though and he's been gone a while.
Yep, I also tend to use hex when I really have a need to deal with bits.
I agree that rewriting it would be ideal but like I said it's "really old code that would be done differently given the time to rewrite it". We have to fix the bug but we can't afford to make major changes at this point for various reasons that I won't get into.
We can and will rewrite it for our next major release though.
It took a while to find the commit because I decided to start looking in the middle of history instead at the beginning.
I didn't do it. It wasn't done by the guy that I was thinking about either.
Aside, I stopped reading DailyWTF a while ago. At a certain point it all starts to sound like "...blah blah blah, new job, blah blah blah, no VCS/bug tracker, blah blah blah legacy code..." and the occasional code example where someone wrote a 25 level of cascading if/else chain. Eventually, this became boring.