The Daily WTF: Curious Perversions in Information Technology

2010-08-25 Reply Admin

TRWTF is another language without a while loop.

2010-08-25 Reply Admin

I've actually done this with a do{}while(0) loop in C, because it was clearer than the alternatives. (This was generated code, btw; the generated version was meant to be readable, but the original was the bit you actually edited.) It's a nice paradigm for what would otherwise lead to 20 nested if statements.

2010-08-25 Reply Admin

If you want that kind of jumpy behavior, refactor into a method and use returns;

... bool bCreateModel = shouldCreateModel(); ... private boolean shouldCreateModel() { if (!pModel) { return true; }

if (asModelParts.GetSize() != asModelPartsToLoad.GetSize())
{
    return true;
}

for (UINT32 i = 0; i < asModelPartsToLoad.GetSize(); ++i)
{
    if (asModelPartsToLoad[i] != asModelParts[i])
    {
        return true;
    }
}

return false;

}

2010-08-25 Reply Admin

The "break;" as the last statement in the loop is like the punchline at the end of a joke.

2010-08-25 Reply Admin

Another example of "Stupid Language Tricks".

Wrongfellow · 2010-08-25 Reply Admin

I've seen this kind of construct a few times. The worst one consisted of three nested "loops" and spanned about seven pages of code.

Usually these things are produced by people who've read that "GOTO makes code unreadable" and started mindlessly parroting it without really understanding it.

But in this case TRWTF is that it never loops at all and can easily be rewritten as an "if(...) { ... } else if(...) { ... }" block without any breaks or faux-loops at all.

(Of course, maybe this code was written for an embedded system which doesn't have an "else" keyword.)

2010-08-25 Reply Admin

If you want that kind of jumpy behavior, refactor into a method and use returns;
... bool bCreateModel = shouldCreateModel(); ... private boolean shouldCreateModel() { if (!pModel) { return true; }

if (asModelParts.GetSize() != asModelPartsToLoad.GetSize()) { return true; }

for (UINT32 i = 0; i < asModelPartsToLoad.GetSize(); ++i) { if (asModelPartsToLoad[i] != asModelParts[i]) { return true; } }

return false; }

but that wouldn't reproduce the original behaviour. Try private boolean shouldCreateModel() { if (!pModel) { return true; }

if (asModelParts.GetSize() != asModelPartsToLoad.GetSize()) { return true; }

boolean retval = false; for (UINT32 i = 0; i < asModelPartsToLoad.GetSize(); ++i) { if (asModelPartsToLoad[i] != asModelParts[i]) { retval = true; } }

return retval }

you have to make sure you complete the inner loop, not jump out of it early

2010-08-25 Reply Admin

bool bCreateModel = false;
if (!pModel || asModelParts.GetSize() != asModelPartsToLoad.GetSize())
{
    bCreateModel = true;        
}

for (UINT32 i = 0; i < asModelPartsToLoad.GetSize() && !bCreateModel; ++i)
{
    if (asModelPartsToLoad[i] != asModelParts[i])
    {
        bCreateModel = true;            
    }
}

Fixed?

2010-08-25 Reply Admin

This looks like C++. Why not just use the goto keyword?

2010-08-25 Reply Admin

The real WTF is you and the people responsible for firing this guy. He is clearly the only one who knows his job.

This is the best one can do without using a goto which you probably don't even have in whatever bastard language that is.

2010-08-25 Reply Admin

No. Now the for loop that goes through all the slow operations of checking each part gets run each time.

2010-08-25 Reply Admin

CodeRage:
If you want that kind of jumpy behavior, refactor into a method and use returns;
... bool bCreateModel = shouldCreateModel(); ... private boolean shouldCreateModel() { if (!pModel) // global variable??? { return true; }
if (asModelParts.GetSize() != asModelPartsToLoad.GetSize()) <b>// global variables???</b>
{
    return true;
}

for (UINT32 i = 0; i < asModelPartsToLoad.GetSize(); ++i)
{
    if (asModelPartsToLoad[i] != asModelParts[i])
    {
        return true;
    }
}

return false;
}

toth · 2010-08-25 Reply Admin

the article:
Thankfully, the fellow responsible is 'no longer with us'

I hope that means he was executed.

Cbuttius · 2010-08-25 Reply Admin

With regards to the code I would prefer to:

Get all that code into a function and return true or false, not break with some variable set.
Don't use Hungarian notation
Use a standard C++ container. They don't have methods called GetSize()
Use std::equal for the later loop rather than a handwritten loop.

something like this:

bool ModelHolder::mustCreateModel() const
{
   return !model 
    || modelPartsToLoad.size() != modelParts.size() 
    ||    !std::equal( modelPartsToLoad.begin(), modelPartsToLoad.end(), modelParts.begin() );
}

2010-08-25 Reply Admin

A Tester:
If you want that kind of jumpy behavior, refactor into a method and use returns; [snip]
but that wouldn't reproduce the original behaviour. Try [snip] you have to make sure you complete the inner loop, not jump out of it early

Did you not see the break; in the inner loop in the original code?

2010-08-25 Reply Admin

Other than having an infinite loop

for (;;)

I see little wrong. As others have suggested, a better looping structure for this type of construct is:

do { ... } while (0);

Yes sometimes

goto

is evil, but this is a nice way of doing things if you have been told not to use them. for the others who insist, Fortran 66 is available in some compatibility mode, be my guest.

2010-08-25 Reply Admin

Alex:
No. Now the for loop that goes through all the slow operations of checking each part gets run each time.

Why? Is it for loops or boolean operators that work differently in C++ than they do in Java?

The Nerve:

for (UINT32 i = 0; i < asModelPartsToLoad.GetSize() && !bCreateModel; ++i)

2010-08-25 Reply Admin

A Tester:
If you want that kind of jumpy behavior, refactor into a method and use returns;
... bool bCreateModel = shouldCreateModel(); ... private boolean shouldCreateModel() { if (!pModel) { return true; }

if (asModelParts.GetSize() != asModelPartsToLoad.GetSize()) { return true; }

for (UINT32 i = 0; i < asModelPartsToLoad.GetSize(); ++i) { if (asModelPartsToLoad[i] != asModelParts[i]) { return true; } }

return false; }
but that wouldn't reproduce the original behaviour. Try private boolean shouldCreateModel() { if (!pModel) { return true; }

if (asModelParts.GetSize() != asModelPartsToLoad.GetSize()) { return true; }

boolean retval = false; for (UINT32 i = 0; i < asModelPartsToLoad.GetSize(); ++i) { if (asModelPartsToLoad[i] != asModelParts[i]) { retval = true; } }

return retval }

you have to make sure you complete the inner loop, not jump out of it early

Why complete the inner loop? The CodeSOD breaks out of the inner loop as soon as it sets the Boolean to true, and A Tester does the same thing by returning true when the condition is met. In fact, your code completes the loop unnecessarily. As soon as you set the return value to true, each iteration afterwards is a no-op. Either the condition is false and the return value stays the same, or the condition is true and you set an already-true variable to true. In the end, it accomplishes the same thing with potentially more steps.

2010-08-25 Reply Admin

toth:
the article:
Thankfully, the fellow responsible is 'no longer with us'

I hope that means he was executed.

This code is exactly what’s wrong with C++! He is dragging down the whole system with his idiocy! I insist that his fingers are cut from his hands completely!

2010-08-25 Reply Admin

For everyone who has had the "GoTo is Evil" Kool-Aid forced down their throats, I'll repeat my mantra: "No language feature is evil. It is the misuse of a language feature that is evil."

2010-08-25 Reply Admin

JJ:
For everyone who has had the "GoTo is Evil" Kool-Aid forced down their throats, I'll repeat my mantra: "No language feature is evil. It is the misuse of a language feature that is evil."

Or maybe sometimes the legitimate use? I submit the following for consideration.

HashMap<String, ArrayList<String>> map = new HashMap<String, ArrayList<String>>();

(Yes, I know this doesn't work in C++, but somehow the Java parser knows how to handle the >>)

2010-08-25 Reply Admin

by:
The "break;" as the last statement in the loop is like the punchline at the end of a joke.

Without it, you would have an infinite loop if the first two conditions were false.

On the other hand, the original coder apparently never heard of "else".

Cbuttius · 2010-08-25 Reply Admin

Michael Cox:
This looks like C++. Why not just use the goto keyword?

You're fired

2010-08-25 Reply Admin

loop:
   for (post = first; post.name == "Darth*" || post.name == "*Gland*"; post = post.next)
      goto head_esplode;

head_esplode:  head_esplode();
goto loop;

You'd think your head could only esplode once per Darth GlandStorm post. Not so. In the words of Edsgar Dijkstra, "Two or more, use a hammer." Or something like that.

2010-08-25 Reply Admin

JJ:
For everyone who has had the "GoTo is Evil" Kool-Aid forced down their throats, I'll repeat my mantra: "No language feature is evil. It is the misuse of a language feature that is evil."

Right, just like "guns don't kill people, people kill people". Of course, a having a tool that is specifically designed for killing people certainly makes the person's job a lot easier. But yeah, guns don't kill people and GOTO is a perfectly valid high-level language construct.

2010-08-25 Reply Admin

by:
The "break;" as the last statement in the loop is like the punchline at the end of a joke.

Yes it certainly makes me want to punch the idiot who wrote that mess!

2010-08-25 Reply Admin

I came across a construct absolutely identical to this today.

On a whim, I hunted through the codebase for more examples - and found plenty. All written by people still at the company, in fact, three out of the four people on my team (the fourth being me).

DailyWTF CodeSOD accurately describes my daily work? FML. :-(

2010-08-25 Reply Admin

No, you guys don't get it. This loop works like the "slingshot" maneuver that probes do to pick up speed before leaving orbit. Using a loop and then breaking out of it actually makes the code run faster!

2010-08-25 Reply Admin

Edward von Emacs:

loop:
   for (post = first; post.name == "Darth*" || post.name == "*Gland*"; post = post.next)
      goto head_esplode;
head_esplode:  head_esplode();
goto loop;

Maybe that should be something like `matches(post.name, "Darth*") ...'

Just making this stuff up as I go.

Cbuttius · 2010-08-25 Reply Admin

goto is entirely inappropriate here though, just a few || operators.

subanark · 2010-08-25 Reply Admin

I guess C doesn't have labeled breaks? E.g.

Label:
{
   ...
   break Label;
   ...
}

danixdefcon5 · 2010-08-25 Reply Admin

Edward von Emacs:
Edward von Emacs:
loop:
   for (post = first; post.name == "Darth*" || post.name == "*Gland*"; post = post.next)
      goto head_esplode;
head_esplode:  head_esplode();
goto loop;
Maybe that should be something like `matches(post.name, "Darth*") ...'

Just making this stuff up as I go.

Your code was successful. My head exploded after reading it. ;)

2010-08-25 Reply Admin

Michael Cox:
This looks like C++. Why not just use the goto keyword?

We found a witch, may we burn him?

2010-08-25 Reply Admin

vydf:
Michael Cox:
This looks like C++. Why not just use the goto keyword?

We found a witch, may we burn him?

I'm not really comfortable with you burning Mike Cox.

Maurits · 2010-08-25 Reply Admin

Meh. for (;;) { ... break; } is equivalent to do { ... } while(0);, which is one of three common memes in the code base I work in.

MEME 1, goto:

    hr = GetResource(&r);
    if (FAILED(hr)) { goto Cleanup; }
    ...

Cleanup:
    FreeResource(r);
    return hr;

MEME 2, do { ... } while(0);

    do {
        hr = GetResource(&r);
        if (FAILED(hr)) { break; }
        ...
    } while(0);

    FreeResource(r);
    return hr;

MEME 3, RAII:

    SmartResourceWrapperObject r;
    hr = GetResource(&r);
    if (FAILED(hr)) { return hr; }
    ...
    (FreeResource called in ~SmartResourceWrapperObject)

As far as I can tell, none of these is inherently superior to the others.

2010-08-25 Reply Admin

GOTO may not be evil, but the "COMEFROM" statement (bonus points for the first person to identify the late 1970's language that had it) was pure evil.

It basically acted as a interceptor so that when the specified item was about to execute control would be transfered to the specified target, which could then decide if it was to return to the original statement or the one after it. Making things even more insane was the ability to have these in conditionals!

2010-08-25 Reply Admin

Someone needs to give that fellow a copy of the famous paper "For Loop Considered Harmful: A Retrospective".

2010-08-25 Reply Admin

GOTO may not be evil, but the "COMEFROM" statement (bonus points for the first person to identify the late 1970's language that had it) was pure evil.

It basically acted as a interceptor so that when the specified item was about to execute control would be transfered to the specified target, which could then decide if it was to return to the original statement or the one after it. Making things even more insane was the ability to have these in conditionals!

Cbuttius · 2010-08-25 Reply Admin

The real WTF is that if the arrays are not the same size or don't match it should return true, and if they match it should return false, but if the pointer is NULL it should return fileNotFound.

pitchingchris · 2010-08-25 Reply Admin

TheCPUWizard:
GOTO may not be evil, but the "COMEFROM" statement (bonus points for the first person to identify the late 1970's language that had it) was pure evil.

From what I'm reading, it was intended to be an april fools joke and never got removed.

Cbuttius · 2010-08-25 Reply Admin

comefrom would be very useful in C++, especially when catching an exception.

2010-08-25 Reply Admin

What is it with the weird for loop abusing lately? Whatever happened to the while loop when you are not iterating over an array or collection but you need to loop anyway?

And to think some people actually blame java itself for this kind of crap code.

frits · 2010-08-25 Reply Admin

Cbuttius:
comefrom would be very useful in C++, especially when catching an exception.

Sounds like you need to implement a stack trace.

2010-08-25 Reply Admin

Java has the nice, largely unused feature of labeled blocks, which can be used in these cases (when you're too lazy extract the logic to another method).

determineCreate: {
    if (!pModel)
    {
        bCreateModel = true;
        break determineCreate;
    }
    ...
}

2010-08-25 Reply Admin

NASA Engineer:
No, you guys don't get it. This loop works like the "slingshot" maneuver that probes do to pick up speed before leaving orbit. Using a loop and then breaking out of it actually makes the code run faster!

Lovely. I'm stealing this for future use!

2010-08-25 Reply Admin

by:
vydf:
Michael Cox:
This looks like C++. Why not just use the goto keyword?
We found a witch, may we burn him?
I'm not really comfortable with your burning Mike Cox.

FTFY

2010-08-25 Reply Admin

NASA Engineer:
No, you guys don't get it. This loop works like the "slingshot" maneuver that probes do to pick up speed before leaving orbit. Using a loop and then breaking out of it actually makes the code run faster!

You have to give the compiler something to optimize away if you really want the maximum possible performance.

2010-08-25 Reply Admin

This is actually a pretty common coding convention. It is not an elaborate GOTO but a poor man exception handling.

It is usually used when you have many initializations intermixed with things that can fail and you want to be able to write all the cleanup code in one place (which was NOT the situation in the example).

It is better written with a "do {} while (0);".

This is not my style of coding, and in the specific code example it was not warranted at all. Yet this is a valid style and certainly not deserving a WTF.

newfweiler · 2010-08-25 Reply Admin

TheCPUWizard:
GOTO may not be evil, but the "COMEFROM" statement (bonus points for the first person to identify the late 1970's language that had it) was pure evil.
It basically acted as a interceptor so that when the specified item was about to execute control would be transfered to the specified target, which could then decide if it was to return to the original statement or the one after it. Making things even more insane was the ability to have these in conditionals!

I remember the proposal to add COMEFROM, COMEFROM DEPENDING ON, (computed COMEFROM) and conditional COMEFROM to COBOL and ForTran.

The modern language that has COMEFROM is Java, only it's called "Annotations". It's really a COMESUB rather than a COMEFROM, and duplicate labels are allowed.

It's useful for application platforms where you want to call your own method from platform code, without modifying the platform code to add a line to call your method.

For example:

public class CustomerService {
   public Response getCustomer(String id) {
     COMEFROM (HttpRequestProcessor) 
        WHEN (method.equals("GET") 
&& path.startsWith("/app/customer");
     // construct response here
     Response resp = ...;
     GOBACK(resp);
   }
}

Actual code:

@Path("/app/customer/{id}")
public class CustomerService {
  @GET
  public Response getCustomer(@PathParam("id") String id) {
    // Construct response here
    Response resp = ...;
    return (resp);
  }
}

newfweiler · 2010-08-25 Reply Admin