• Larry (unregistered)

    TRWTF is another language without a while loop.

  • 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.

  • 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;
    

    }

  • by (unregistered)

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

  • Crash Magnet (unregistered)

    Another example of "Stupid Language Tricks".

  • (cs)

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

  • A Tester (unregistered) in reply to CodeRage
    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

  • 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?

  • Michael Cox (unregistered)

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

  • 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.

  • Alex (unregistered) in reply to The Nerve

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

  • The Nerve (unregistered) in reply to CodeRage
    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;
    

    }

  • (cs)
    the article:
    Thankfully, the fellow responsible is 'no longer with us'

    I hope that means he was executed.

  • (cs)

    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() );
    }
    
  • Carl T (unregistered) in reply to A Tester
    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?
  • 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.

  • The Nerve (unregistered) in reply to Alex
    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)
  • Mad Adder (unregistered) in reply to A Tester
    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.

  • Bert Glanstron (unregistered) in reply to toth
    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!

  • 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."

  • The Nerve (unregistered) in reply to JJ
    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 >>)
  • Ken B. (unregistered) in reply to by
    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".

  • (cs) in reply to Michael Cox
    Michael Cox:
    This looks like C++. Why not just use the goto keyword?

    You're fired

  • 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.

  • Anonymous (unregistered) in reply to JJ
    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.
  • plaga (unregistered) in reply to by
    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!
  • 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. :-(

  • 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!

  • Edward von Emacs, VI (unregistered) in reply to Edward von Emacs, VI
    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.

  • (cs)

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

  • (cs)

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

    Label:
    {
       ...
       break Label;
       ...
    }
    
  • (cs) in reply to Edward von Emacs, VI
    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. ;)

  • vydf (unregistered) in reply to Michael Cox
    Michael Cox:
    This looks like C++. Why not just use the goto keyword?

    We found a witch, may we burn him?

  • by (unregistered) in reply to vydf
    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.

  • (cs)

    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.

  • 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!

  • PsyberS (unregistered)

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

  • 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!

  • (cs) in reply to by

    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.

  • (cs) in reply to TheCPUWizard
    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.

  • (cs)

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

  • 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.

  • (cs) in reply to Cbuttius
    Cbuttius:
    comefrom would be very useful in C++, especially when catching an exception.

    Sounds like you need to implement a stack trace.

  • 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;
        }
        ...
    }
    
  • wtf (unregistered) in reply to NASA Engineer
    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!

  • FuBar (unregistered) in reply to by
    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
  • Fred (unregistered) in reply to NASA Engineer
    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.
  • 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.

  • (cs) in reply to TheCPUWizard
    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);
      }
    }
    
  • (cs) in reply to TheCPUWizard
    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);
      }
    }
    

Leave a comment on “Masquerading as a Loop”

Log In or post as a guest

Replying to comment #:

« Return to Article