Their programs control your bank account

April 27, 2005 1:08 AM

DailyWTF is a "Programming Bloopers" repository and forum, collecting, dissecting and making good fun of badly written code. Programmers can appreciate their fellow coders' strange or plainly funny problem solving techniques. Sometimes programmers will square the wheel while reinventing it. Or take the best practices to the insanity level.
Some programming knowledge required.
posted by nkyad (21 comments total)

Holy shit. WTF doesn't even cover it. The programmers for the first and fourth links in particular ought to be strung up and shot through the lungs.
posted by Ryvar at 1:43 AM on April 27, 2005

[this is good]
posted by mek at 1:54 AM on April 27, 2005

WTF?
posted by grouse at 2:09 AM on April 27, 2005

Haha, thanks for the dupe link, grouse. Almost as funny as the site itself is all of the "I'm not a programmer, help!" comments in that thread.
posted by Plutor at 2:56 AM on April 27, 2005

Glad this was re-posted as, when it was first up, I wasn't a member and couldn't comment. I used to work for these guys (the company is now dead and this wasn't me!)
posted by TheDonF at 4:42 AM on April 27, 2005

Is this something I'd have to use a computer to understand?
posted by sourwookie at 7:15 AM on April 27, 2005

Sorry for the dupe - I searched for "www.thedailywtf.com" and found nothing, but the url in the previous post didn't include the www.
posted by nkyad at 7:37 AM on April 27, 2005

One of my favorite Usenet threads (especially the last post).
posted by Armitage Shanks at 7:50 AM on April 27, 2005

sourwookie writes "Is this something I'd have to use a computer to understand?"

You'd have to understand programming.

Well, maybe not. Programs are just a sequence of instructions to the computer.

If you l look at the first link, you'll notice how much duplication of code there is. Or wait, I'll do it for you. Duplicate code in italics, non duplicated in boldface.

do
{
if (entry_is_valid)
{
BOOL discoveredEntry;
RemoveEntry(List);
discoveredEntry = !IsListEmpty(List)

process_valid_entry();

if (discoveredEntry)
goto DoNextListItem;
else
break;
}
else
{
BOOL discoveredEntry;
RemoveEntry(List);
discoveredEntry = !IsListEmpty(List)

process_notvalid_entry();

if (discoveredEntry)
goto DoNextListItem;
else
break;
}
ASSERT(FALSE); // Shouldn't get here.
DoNextListItem:
} while (!IsListEmpty(List));

Ok, first, let's factor out the duplication:
do
{
BOOL discoveredEntry;
RemoveEntry(List);
discoveredEntry = !IsListEmpty(List)

if (entry_is_valid) {
process_valid_entry();
}
else {
process_notvalid_entry();
}

if (discoveredEntry)
goto DoNextListItem;
else
break;
}
ASSERT(FALSE); // Shouldn't get here.
DoNextListItem:
} while (!IsListEmpty(List));

Now note the assertion. assertions should only be used to discover that something is wrong at run-time. But the code is written such that the assertion can never be reached, so its use is utterly pointless.

Note also his goto: it jumps to the end of a while loop, and the condition controlling whether the got: is reached is the same condition controlling the while loop. It's another, slightly less obvious duplication of code. Look closely, and you'll see that the value of discoveredEntry is set to be the expression !IsListEmpty(List), and the while loop is controlled by that exact same expression. So we re-factor again:

do
{
BOOL discoveredEntry;
RemoveEntry(List);
discoveredEntry = !IsListEmpty(List)

if (entry_is_valid) {
process_valid_entry();
} else {
process_notvalid_entry();
}
}
} while ( discoveredEntry );

That's as far as we can go without knowing if the process_valid_entry and process_notvalid_entry functions can change the value returned by IsListEmpty. They probably don't, as the List functions all take a (presumably) pointer, the "List" parameter. But since the code sucked so badly to begin with, we have to be pessimistic and wonder if a pointer to the List is hardcoded in either of these functions. But if they can't change that value, we can simplify further:

do
{
RemoveEntry(List);

if (entry_is_valid) {
process_valid_entry();
} else {
process_notvalid_entry();
}
}
} while ( !IsListEmpty(List) );

Finally, we can make some minor syntactical changes that don't affect what we're asking the computer to do, but make the code easier for humans to read:

do {
RemoveEntry(List);
entry_is_valid ? process_valid_entry() : process_notvalid_entry() ;
} while ( !IsListEmpty(List) );

I'd go a bit further, myself, and write a forwarding function to unify processing entries, this is slightly slower but it presents a uniform interface:


void process_entry( bool valid ) {
valid ? process_valid_entry() : process_notvalid_entry() ;
}

Then the main code fragment would be:

do {
RemoveEntry(List);
process_entry( entry_is_valid ) ;
} while ( !IsListEmpty(List) );
posted by orthogonality at 8:07 AM on April 27, 2005

LINE 10 PRINT "HELLO WORLD"
LINE 20 GOTO 30
posted by Smart Dalek at 8:37 AM on April 27, 2005

try running this batch file:
:foo
copy %0>>%0
:
posted by sonofsamiam at 8:43 AM on April 27, 2005

Like a comedian explaining the joke, orthogonality, you've just silenced the room.
posted by xmutex at 9:02 AM on April 27, 2005

Actually, it looked a lot more like a College Professor explaining the joke to the class.
posted by nkyad at 9:31 AM on April 27, 2005

I read orthogonality's explanation as taking place in one of Mr. Kerber's classes in Better Off Dead. Did wonders for the whole effect...
posted by Fezboy! at 10:32 AM on April 27, 2005

Especially since he was responding to a joke question.
posted by gummo at 10:47 AM on April 27, 2005

Mixing underscored_method_names() with camelCasedMethodNames() should be a firing offense.
posted by Armitage Shanks at 10:47 AM on April 27, 2005

while(eat_shit) {
     bark_at_moon;
}
posted by quonsar at 11:20 AM on April 27, 2005

er, bark_at_moon();
posted by quonsar at 11:22 AM on April 27, 2005

We have some code here that I should submit. Someone decided that they needed to use constants for every literal value. So they created them:
const integer ciZERO = 0
const integer ciONE = 1

So, in case we ever need to change the value of 0 in all of our code, we can just change ciZERO to equal 1. That will lead to easily readable code for sure!

Note that they only got up to ciFIVE- I guess they never wrote a loop that needed to go more than 5 explicit iterations.
posted by Four Flavors at 11:38 AM on April 27, 2005

Lord all-mighty.... everyone at work had quite a laugh at some of these examples.

This one is my favorite:

bool IsNegative(double n)
{
string nStr = n.ToString();
if (nstr.IndexOf('-', 0, 1)) return true;
return false;
}

You think it'd be easier to tell if something was less than zero, wouldn't you? :) Thanks for the great post nkyad!
posted by gambit at 5:30 PM on April 27, 2005

How To Write Unmaintainable Code. 'Nuff said.
posted by Lush at 12:11 AM on April 29, 2005