Comment On From WTF to -ism

Anonymous:

That ASSERT is priceless.

Especially because of the comment behind it, because the writer partially sees how dumb he is (but it makes him look even dumber :P).

My favorite part is the fact that he's doing a do loop instead of a
while so he has to check for an empty list inside (why he didn't just
check it before entering the loop is another WTF).  If he just
changed that around to:

while (!IsListEmpty(List))

then he wouldn't need

if (discoveredEntry)
   goto DoNextListItem;
else
   break;

Of course the real WTF here is the goto since all it serves to do is jump him past his own ASSERT!

OK, so does anyone see any reason why the follow should work the same way:

do
{
    BOOL is_valid = entry_is_valid;
    BOOL discoveredEntry;

    RemoveEntry(List);
    discoveredEntry = !IsListEmpty(List)

  if (is_valid)
    process_valid_entry();
  else
    process_notvalid_entry();

} while (discoveredEntry);

It maintains every possible limitation I was able to derive from the original code, namely:

1. It could be C code, and require all declarations before any executable statement in a block,


2. Calling process_{not}valid_entry may change the return value of IsListEmpty, and


3. Calling RemoveEntry could change the value of (the presumably global) entry_is_valid

I actually, doesn't think 1 & 2 are actual requirements, but can't tell without the rest of the source code (#3, on the other hand, I feel is quite legitimate).  If we remove them, we get:

do
{
    BOOL is_valid = entry_is_valid;
    RemoveEntry(List);
  if (is_valid)
    process_valid_entry();
  else
    process_notvalid_entry();

} while (!IsListEmpty(List));

EsotericMoniker:

My favorite part is the fact that he's doing a do loop instead of a
while so he has to check for an empty list inside (why he didn't just
check it before entering the loop is another WTF).  If he just
changed that around to:

while (!IsListEmpty(List))

then he wouldn't need

if (discoveredEntry)
   goto DoNextListItem;
else
   break;

Of course the real WTF here is the goto since all it serves to do is jump him past his own ASSERT!

Also he checks it in the while condition at the end, so the inner if isn't necessary, even with the rest of the stupid crap here.

I don't understand the thought pattern that leads to this kind of code.  Was this guy perhaps in circulation from before structured programming became the dogma?

EsotericMoniker:

My favorite part is the fact that he's doing a do loop instead of a
while so he has to check for an empty list inside

Actually, that has nothing to do with it.  The if/goto is completely supurfluous.  Presumably, he would have reached this code, unless there was at least one entry to work on.

dubwai:

What I really don't get about this is that there is so much more work (and thought) involved to do this intead of the obvious and short way.

dubwai:

What I really don't get about this is that there
is so much more work (and thought) involved to do this intead of
the obvious and short way.  Is it just me or do some programmers
think that writing arcane code makes them better programmers? 
Like it's a one-up for them if someone struggles to get their head
around their code regardless of how mundane it really is.  I'll be
honest, it took me a minute to figure out just what the hell this
code was doing and I have to maintain garbage code all the
time.

Charles Nadolski:

I meant several hundred, not several thousand.  My bad.

There is a single class in our source code that is just under 10,000 lines.  I shit you not.

dubwai:

Charles Nadolski:

I meant several hundred, not several thousand.  My bad.

There is a single class in our source code that is just under 10,000 lines.  I shit you not.

I came across something similar while maintaining an old Oracle Forms 4.5 trigger - there was so much code in the one trigger that if you tried to add one more line of code it would not compile because it was too large! [:|]

Anonymous:

This is my favorite kind of WTF: the time-release WTF. Like the above poster, I had to stare for a while before I saw what the author was trying to do, and then came a steady stream of realizations as to how bad this code really is. It doesn't hit you all at once; it's like drinking fine whiskey instead of chugging shots.

++ Quoted for truth.[Y]

dubwai:

What I really don't get about this is that there is so much more work (and thought) involved to do this intead of the obvious and short way. 

The truth is, some people were just never meant to be programmers.  The "obvious and short" way isn't obvious to them.   They usually grab some sample code from a related project and hack it into something that they think will do the task.  When it fails, they have no idea why, and hack at it some more.  When they stumple upon something that works, they still don't know why, so they can't clean it up --- they just leave it and hope it doesn't break.  

JamesCurran:

The truth is, some people were just never meant to be
programmers.  The "obvious and short" way isn't obvious to
them.   They usually grab some sample code from a related
project and hack it into something that they think will do the
task.  When it fails, they have no idea why, and hack at it some
more.  When they stumple upon something that works, they still
don't know why, so they can't clean it up --- they just leave it and
hope it doesn't break.  

JamesCurran:

The truth is, some people were just never meant to be programmers. The
"obvious and short" way isn't obvious to them. They usually grab some
sample code from a related project and hack it into something that they
think will do the task. When it fails, they have no idea why, and hack
at it some more. When they stumple upon something that works, they
still don't know why, so they can't clean it up --- they just leave it
and hope it doesn't break.

Anonymous:

This is my favorite kind of WTF: the time-release WTF. Like the above
poster, I had to stare for a while before I saw what the author was
trying to do, and then came a steady stream of realizations as to how
bad this code really is. It doesn't hit you all at once; it's like
drinking fine whiskey instead of chugging shots.

A routine part of my daily decompression is the scotch, whiskey, vodka, and/or brandy.  I've had 3 of the 4 tonight, and I didn't even get around to writing a single line of code today.

At least on my current project (creating a new api to replace a scripted direct-database accesss process), I was able to convince everyone to drastically overhaul the data model. This involves being able to drop roughly 2/3s of the tables involved as being functionally redundant.  [Un]fortunately, the person that first did the data model is no longer with the company.  I suppose if they were, at least I'd have someone to throttle. 

Actually the ASSERT isn't that stupid.  I've seen BOOLs and bools that weren't bools, but a sort of Tri-state something.  I've even seen if(statement){} executing the if if the statement returned false. 

The rest of code well, wtf :-)

Does this code actually do what was "intended" (sorry about the quotes, but I am finding it difficult to tell exactly what WAS intended)

For this code to work (and bear with me, I am struggling to know what working means for this), the following assumptions have to be valid.

1) "entry_is_valid" must be a routine which inspects the item at the head of the list. having a reference to the list available via global vars (or possibly as an instance or class variable).

2) "RemoveEntry" must take the top entry and store a reference or pointer to this somewhere. and in the process dispose of any entry that is already stored in this reference/pointer

3) process_valid_entry and process_nonvalid_entry must access this reference/pointer as their argument

There is such a mix of ways of passing variables and handling items that this is less than clear

Naming conventions are not consistent, this clouds the issue. Specifically, I have assumed that "entry_is_valid" is a routine as it is updated nowhere, but the use of "routine()" calls makes me wonder whether the "enter_is_valid" is a variable that is updated somewhere else.

Before you can even think of fixing this code, you have to alter the routines and functions it calls to work on a consistent basis. This will assist the rest of the program by reducing reliance on globals and side-effects.

Having said all of the above, and taking into account some refactoring of the other routines, I would say that a neater way to implement this would be...

(dunno what language this code is in, so please forgive any problems with references/pointers etc)

while (!IsListEmpty(List))
{
    item anItem;

    anItem = removeEntry(List);

    if entryIsValid(anItem)
    {
        processValidEntry(anItem);
    }
    else
    {
        processInvalidEntry(anItem);
    }

    destroy(anItem);
}

Please correct me if I am wrong, or there is a neater way...

Don't even know if the above code needs comments. maybe some to say WHAT it is doing and WHY it is doing it, but the HOW would seem to be self explanetory.

Anonymous:

Whoever finds him first, please give him a good slapping on behalf of all namesakes.



Cheers,

Rik

V.:

Actually the ASSERT isn't that stupid.  I've
seen BOOLs and bools that weren't bools, but a sort of Tri-state
something.  I've even seen if(statement){} executing the if if the
statement returned false. 

 

The rest of code well, wtf :-)

dhromed:

I don't claim to know all languages in existence, but what kind of language has an uppercase BOOL keyword?

My recollection (which may or may not be correct) is that Visual C++ didn't get the intrinsic 'bool' type until VC++ 5 (I'm assuming that the code was written using VC++).

It wasn't until quite late in the lifetime of the C++ language that the concept of a true bool type (if you'll forgive the pun) was added - this was not exclusively a Microsoft problem.

I have distinct memories of using

#define TRUE (1==1)
#define FALSE (1==0)
#define BOOL int

in some of my early C code (20+ years ago)

Of course, premature senility may be kicking in and I could be talking bollocks.[8-|]

My recollection (which may or may not be correct) is that Visual C++ didn't get the intrinsic 'bool' type until VC++ 5

To Clarify a bit, the C++ language didn't get the intrinsic bool type to about the time of VC5  (It was proposal to the standards committee in 1995, and the Standard itself wasn't approved until 1998)

However, the most common uses of the "BOOL" type is in the Windows APIs which for legal reasons (yes, really, legal reasons), must be compatible with C code (which has never had an intrinsic bool type)

V.:

Actually the ASSERT isn't that stupid.  I've seen BOOLs and bools that weren't bools, but a sort of Tri-state something. 

No, it is stupid.  If you really wanted to check for that condition, then it should have been done explicitly, as :

ASSERT(entry_is_valid == TRUE || entry_is_valid == FALSE);

at the very top.  "ASSERT(FALSE)" pretty much says that you don't know how to code -or- use ASSERT.

JamesCurran:

V.:

Actually the ASSERT isn't that stupid.  I've seen BOOLs and bools that weren't bools, but a sort of Tri-state something. 

No, it is stupid.  If you really wanted to check for that condition, then it should have been done explicitly, as :

ASSERT(entry_is_valid == TRUE || entry_is_valid == FALSE);

at the very top.  "ASSERT(FALSE)" pretty much says that you don't know how to code -or- use ASSERT.

And you've turned an ASSERT(FALSE) into an ASSERT(TRUE), which would never actually do anything useful, regardless of where it was placed.  The point of the assert was to guarantee the loop would break, one way or another.  Its completely unnecessary, however.  Some languages/compilers would likely warn on that code, because of unreachable code.

Of course, if that was really necessary, I'd prefer something like:
throw Exception("Stupid compiler.  Shouldn't be here." );

[:P]

do

{



    BOOL is_valid = entry_is_valid;



    RemoveEntry(List);



    (is_valid ? process_valid_entry : process_notvalid_entry)();



} while (!IsListEmpty(List));

mattparkins:

That post above was meant to be in reply to the 10,000 lines of code in a single source file shocker.

30,000 is pretty long.  In Java you can compile a source file larger that ~65K so 10,000 lines tends to be about as large as you can make a class.  The other thing was that this was no where near being a whole application.  It was just a minor component.  One of at least one-hundred 1000+ line source files.

Anonymous:

And you've turned an ASSERT(FALSE) into an ASSERT(TRUE),

No, because "BOOL", "TRUE" and "FALSE" aren't necessary the same as "bool", "true" and "false".  BOOL is almost certainly typedef'd as an int, and therefore could easily have a value besides "TRUE" or "FALSE". My assert would break on exactly the same condition as the original assert(*), except that mine would a more meaningful error message, and even just in the code, documents the preconditions of the function.

(*) Actually, since the both the if() test the value manifestly, instead of against a particularly value, it is, in fact, completely impossible for the ASSERT(FALSE) ever to be reached, regardless of the values of entry_is_valid and discoveredEntry.

dubwai:

 In Java you can compile a source file larger that ~65K so 10,000 lines tends to be about as large as you can make a class. 

Now, my Java is a bit rusty.  The language does require you to define the entire class in a single file, correct?

C# v1.0 does, but v2.0 won't.  For C++, the declaration (.h file) must be in one file (barring kooky hacks), but the definition (.cpp files) can be split over as many files as needed.

JamesCurran:

dubwai:

 In Java you can compile a source file larger that ~65K so 10,000 lines tends to be about as large as you can make a class. 

Now, my Java is a bit rusty.  The language does require you to define the entire class in a single file, correct?

C# v1.0 does, but v2.0 won't.  For C++, the declaration (.h file) must be in one file (barring kooky hacks), but the definition (.cpp files) can be split over as many files as needed.

Yes, that is correct. Every class must be in a single file.  You can only have one public class per file. Generally people just put all top level classes in their own file even when they are not public for clarity.  The compiler will choke if the source file contains more than 65536 bytes, if my memory serves me correctly.

JamesCurran:

My recollection (which may or may
not be correct) is that Visual C++ didn't get the intrinsic 'bool' type
until VC++ 5

To Clarify a bit, the C++ language didn't get the intrinsic bool
type to about the time of VC5  (It was proposal to the standards
committee in 1995, and the Standard itself wasn't approved until 1998)

However, the most common uses of the "BOOL" type is in the Windows APIs which for legal reasons (yes, really, legal reasons), must be compatible with C code (which has never had an intrinsic bool type)

If having you own -ism isn't just quite good enough for you, you could shoot for the status of the programmer from whom I've inherited much code... There still is the occasional use of -ism's though he has been gone for 6 months. (Actually, there is a set of -ism's) He remains known as 'ED' aka Evil Derek.

j99:

If having you own -ism isn't just quite good enough for you, you could shoot for the status of the programmer from whom I've inherited much code... There still is the occasional use of -ism's though he has been gone for 6 months. (Actually, there is a set of -ism's) He remains known as 'ED' aka Evil Derek.

Alright, make with the WTFs.

Anonymous:

I'm not really a C guy, so please correct me if I'm wrong: Besides the totally flawed code, wouldn't a "continue" have the same effect as that goto directly to the end of the loop??

Other than bypassing a second call to "IsListEmpty()", yes they would be the same....

Ok, one of my favorites is an implied foriegn key relationship from a transaction table to a cardholder table, sometimes... sometimes the foreign key column value corresponds to an account table. kinda just depends on how the code feels at the time, but the best I could figure was it worked like this.

First a cardholder record is created, if the user is new the new cardholderID is used as the SourceID for the tranaction table. If it is determined there is an existing record for that person in the account table, the AccountID is used for the SourceID in the transaction record. (cool, huh?)

It certain situations, a new transaction must be created for for that person based on the previous transaction. It then uses that person's AccountID as the SourceID for the new transaction. Then another process uses that new transaction (picks up all new transactions) to collect fees. Well, that fee collection process assumes the SourceID references a CardHolderID.

Anyone see what's coming here? Yes, sometimes is does not find a person to collect from essentially giving a person free product. But even better than that is that it sometimes find a match in the cardholder table to the value which is supposed to be an accountID and ends up billing the wrong person.