Comment On Masquerading as a Loop

"While digging through some inherited code," writes Joe "M2tM" Smith, "I encountered a conditional masquerading as a loop." [expand full text]
« PrevPage 1 | Page 2 | Page 3Next »

Re: Masquerading as a Loop

2010-08-25 11:00 • by Larry (unregistered)
TRWTF is another language without a while loop.

Re: Masquerading as a Loop

2010-08-25 11:02 • by ais523 (unregistered)
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.

Re: Masquerading as a Loop

2010-08-25 11:05 • by CodeRage (unregistered)
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;
}

Re: Masquerading as a Loop

2010-08-25 11:06 • by by (unregistered)
The "break;" as the last statement in the loop is like the punchline at the end of a joke.

Re: Masquerading as a Loop

2010-08-25 11:08 • by Crash Magnet (unregistered)
Another example of "Stupid Language Tricks".

Re: Masquerading as a Loop

2010-08-25 11:08 • by Wrongfellow
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.)

Re: Masquerading as a Loop

2010-08-25 11:09 • by A Tester (unregistered)
319428 in reply to 319421
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

Re: Masquerading as a Loop

2010-08-25 11:11 • by The Nerve (unregistered)
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?

Re: Masquerading as a Loop

2010-08-25 11:12 • by Michael Cox (unregistered)
This looks like C++. Why not just use the goto keyword?

Re: Masquerading as a Loop

2010-08-25 11:15 • by transverbero (unregistered)
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.

Re: Masquerading as a Loop

2010-08-25 11:16 • by Alex (unregistered)
319435 in reply to 319430
No. Now the for loop that goes through all the slow operations of checking each part gets run each time.

Re: Masquerading as a Loop

2010-08-25 11:17 • by The Nerve (unregistered)
319438 in reply to 319421
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()) // global variables???
{
return true;
}

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

return false;
}

Re: Masquerading as a Loop

2010-08-25 11:17 • by toth
the article:

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


I hope that means he was executed.

Re: Masquerading as a Loop

2010-08-25 11:19 • by Cbuttius
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() );
}

Re: Masquerading as a Loop

2010-08-25 11:22 • by Carl T (unregistered)
319448 in reply to 319428
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?

Re: Masquerading as a Loop

2010-08-25 11:24 • by Herby (unregistered)
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.

Re: Masquerading as a Loop

2010-08-25 11:24 • by The Nerve (unregistered)
319452 in reply to 319435
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)

Re: Masquerading as a Loop

2010-08-25 11:26 • by Mad Adder (unregistered)
319453 in reply to 319428
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.

Re: Masquerading as a Loop

2010-08-25 11:27 • by Bert Glanstron (unregistered)
319455 in reply to 319439
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!

Re: Masquerading as a Loop

2010-08-25 11:31 • by JJ (unregistered)
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."

Re: Masquerading as a Loop

2010-08-25 11:34 • by The Nerve (unregistered)
319463 in reply to 319458
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 >>)

Re: Masquerading as a Loop

2010-08-25 11:34 • by Ken B. (unregistered)
319464 in reply to 319423
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".

Re: Masquerading as a Loop

2010-08-25 11:35 • by Cbuttius
319465 in reply to 319431
Michael Cox:
This looks like C++. Why not just use the goto keyword?


You're fired

Re: Masquerading as a Loop

2010-08-25 11:35 • by Edward von Emacs, VI (unregistered)

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.

Re: Masquerading as a Loop

2010-08-25 11:37 • by Anonymous (unregistered)
319469 in reply to 319458
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.

Re: Masquerading as a Loop

2010-08-25 11:37 • by plaga (unregistered)
319470 in reply to 319423
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!

Re: Masquerading as a Loop

2010-08-25 11:38 • by Andy P (unregistered)
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. :-(

Re: Masquerading as a Loop

2010-08-25 11:38 • by NASA Engineer (unregistered)
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!

Re: Masquerading as a Loop

2010-08-25 11:38 • by Edward von Emacs, VI (unregistered)
319474 in reply to 319466
Edward von Emacs, VI:

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.

Re: Masquerading as a Loop

2010-08-25 11:38 • by Cbuttius
goto is entirely inappropriate here though, just a few || operators.

Re: Masquerading as a Loop

2010-08-25 11:44 • by subanark
I guess C doesn't have labeled breaks? E.g.

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

Re: Masquerading as a Loop

2010-08-25 11:47 • by danixdefcon5
319492 in reply to 319474
Edward von Emacs, VI:
Edward von Emacs, VI:

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. ;)

Re: Masquerading as a Loop

2010-08-25 11:49 • by vydf (unregistered)
319497 in reply to 319431
Michael Cox:
This looks like C++. Why not just use the goto keyword?


We found a witch, may we burn him?

Re: Masquerading as a Loop

2010-08-25 11:50 • by by (unregistered)
319499 in reply to 319497
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.

Re: Masquerading as a Loop

2010-08-25 11:52 • by Maurits
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.

Re: Masquerading as a Loop

2010-08-25 11:54 • by TheCPUWizard (unregistered)
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!

Re: Masquerading as a Loop

2010-08-25 11:55 • by PsyberS (unregistered)
Someone needs to give that fellow a copy of the famous paper "For Loop Considered Harmful: A Retrospective".

Re: Masquerading as a Loop

2010-08-25 12:00 • by TheCPUWizard (unregistered)
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!

Re: Masquerading as a Loop

2010-08-25 12:01 • by Cbuttius
319516 in reply to 319423
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.

Re: Masquerading as a Loop

2010-08-25 12:03 • by pitchingchris
319518 in reply to 319506
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.

Re: Masquerading as a Loop

2010-08-25 12:05 • by Cbuttius
comefrom would be very useful in C++, especially when catching an exception.

Re: Masquerading as a Loop

2010-08-25 12:08 • by jdev (unregistered)
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.

Re: Masquerading as a Loop

2010-08-25 12:08 • by frits
319523 in reply to 319519
Cbuttius:
comefrom would be very useful in C++, especially when catching an exception.


Sounds like you need to implement a stack trace.

Re: Masquerading as a Loop

2010-08-25 12:19 • by dave (unregistered)
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;
}
...
}

Re: Masquerading as a Loop

2010-08-25 12:20 • by wtf (unregistered)
319535 in reply to 319473
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!

Possessive pronoun (genitive) to be used with gerund

2010-08-25 12:24 • by FuBar (unregistered)
319538 in reply to 319499
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

Re: Masquerading as a Loop

2010-08-25 12:25 • by Fred (unregistered)
319539 in reply to 319473
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.

Re: Masquerading as a Loop

2010-08-25 12:28 • by Moshe (unregistered)
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.

Re: Masquerading as a Loop

2010-08-25 12:32 • by newfweiler
319542 in reply to 319506
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);
}
}

Re: Masquerading as a Loop

2010-08-25 12:33 • by newfweiler
319543 in reply to 319506
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);
}
}

« PrevPage 1 | Page 2 | Page 3Next »

Add Comment