Gotos considered harmful, but break considered OK?



  • I've inherited a large C codebase written by someone who had been told, as we all were, that "goto" is bad - but apparently never told why. There's a bunch of places where they want to do a goto, so instead they have a do/while(false) and then just "break" out of the middle - it's exactly like a goto, except that you can't easily find where you'll end up (some blocks are thousands of lines long, and the indentation is all fscked up), and if you're at the end, you can't easily find all the places that could send you there:


    // a zillion lines of code
    do{
    // a zillion more
    if(something_happened)
       break;
    // a zillion more
    }while(false);
    // a zillion more

    WTF???????????



  • Stupid is as stupid does. If the original programmer had refactored the zillion lines of code to shorter methods there would be no need for such odd constructs.



  • There's no need now! If you really need to get from here to there (perhaps you're a maintenance programmer given 1 hr to fix a bug, and you can't afford to refactor anything), just put in a fricken goto! At least it would be clear. But no... goto would be bad!!!!



  • Ironically the one time I tried to use a goto the compiler generated bad code, I think messing up the stack (I suppose I was doing the same thing :) ).  I think because nobody uses them, no one notices if there are associated compiler bugs, so they never get fixed, so they cannot be used.

    Using a break inside a do {} while( false ); loop is just reimplementing goto to avoid being told not to use it.  I was wondering if it would mean that any variables declared in the loop block would not be cleaned up?

    Incidentally do you think the following goto substitute code is just as bad?  Or does the naming of the exception make the jump and destination points sufficiently readable?  It does have the advantage that objects will be cleaned up correctly.

     
    try {

    // do stuff

    if( want to abort )

       throw AbortException;

    // more stuff

    }  catch( AbortException ) {

    }

     



  • This is actually quite common practice, and for good reason.  In C there was no try/catch so this was the cleanest way to write code that had a lot of error handling in it.  The alternative would be to have a bunch of nested if/else statements that could get way out of hand if there were a lot of error conditions.  Or you could do a return out of the function on an error but doing this is considered bad practice by a lot of people since it makes setting breakpoints while debugging all that much harder and it makes it impossible to have cleanup code at the end of the function.  GOTO's are considered bad because they can cause really bad spaghetti code, however if they were used properly just for error conditions they could be a valid alternative to break; on error.



  • WTF? 'Don't use goto' doesn't mean 'do a disgusting nasty kludge to fake goto, except it's even worse because there's no label to search for'! I think do {...} while(false) is a pretty spectacular WTF all on its own!

    The throw/catch type of thing is usually bad design as well, although it's not as bad as this WTF because the exception you throw can contain useful information about why you bogged out. The only time I recommend this type of thing is faking an error condition that could occur in the try block anyway, e.g.:

    try {
     // Pretend we're a real operating system that doesn't like
     // spaces in filenames
     if(filename.IndexOf(' ') >= 0) throw new IOException("Invalid filename "+filename);
     FileReader f = File.Open(filename);
     // ... etc
    } catch(IOException e) {
     ... useful stuff to do if the file operation failed
    }

    (Edit: WTF? <tt> doesn't work in the real page, but it does in the preview? Pre blocks, and strikethrough, vice versa. And my <pre> block got mutilated with <br /> crap when I went to edit the post.)



  • I can't edit my post any longer.  Just to clarify I was suggesting goto might not clean up variables properly, whilst break and throw definitely will.



  • The goto (or lack thereof) in question was in C code (not C++), so object cleanup was not an issue at all.

    In C++, you can get screwed up with goto jumping past a constructor (though my compiler, VC6, gives an error if you try), but it will properly perform the cleanup if a goto jumps out of the block.



  • I knew a fellow who used this pattern because gotos were not allowed by the company:

     
    #include <stdio.h>
    #define skip(x) goto x
    #define mark(x) x:
                                                                                                                 
    int main(int argc, char** argv) {
        printf("line 1\n");
        skip(line3);
        printf("line 2\n");
        mark(line3);
        printf("line 3\n");
                                                                                                                 
        return 0;
    }



  • With a little clever layering of macros, he could even physically separate the "go" and "to" so that a simple grep for "goto" wouldn't find anything!



  • [quote user="MET"]

    Ironically the one time I tried to use a goto the compiler generated bad code, I think messing up the stack (I suppose I was doing the same thing :) ).  I think because nobody uses them, no one notices if there are associated compiler bugs, so they never get fixed, so they cannot be used.

    Using a break inside a do {} while( false ); loop is just reimplementing goto to avoid being told not to use it.  I was wondering if it would mean that any variables declared in the loop block would not be cleaned up?

    Incidentally do you think the following goto substitute code is just as bad?  Or does the naming of the exception make the jump and destination points sufficiently readable?  It does have the advantage that objects will be cleaned up correctly.

     
    try {
    // do stuff
    if( want to abort )
       throw AbortException;
    // more stuff
    }  catch( AbortException ) {
    }[/quote]

    What about the acceptible way of

    // do stuff
    if (do not want to abort) {
        // more stuff

    Oh, and look, it is easy to read, shorter, and makes sense.

    Unless you don't understand negatives.  In which case, You = ! realProgrammer.

     

     



  • This isn't a WTF. He just didn't realize you could put more than one line of code in an if block.

    //code1
    do {
       //code2
       if(something_happened)
          break;
       //code3
    } while(false);
    //code4
    //code1
    

    //code2
    if(!something_happened) {
    //code3
    }
    //code4



  • [quote user="BitTwiddler"]

    WTF???????????

    [/quote]

    Boss: "Bob, program X is crashing. Find out why. Now."

    Bob: [examines the code] "There's not enough error checking."

    Boss: "Fix it. Now."

    Bob: "That will take..."

    Boss: "What part of 'now' don't you understand?"

    Bob: "You got it."

    if(oops) break;

     



  • The WTF in the code was the zilion lines, not the use of break. The things that a lot of (mainly young) people fails to understand is that is not goto in itself which is bad, but the spaghetti code using unwisely produces.

    With goto you can jump everywhere in the code (well, almost everywhere, you can not jump from outside to inside a block). If you do not restrain yourself, the code becomes so tangled none can understand it anymore. On the other side, break brings you just outside the current block, and looking for a closed brace brings you there.

    As someone has remarked, this is mainly useful when you have exceptional (as in Exception) conditions where you should stop processing. Sure, you could do with ifs, like this

    do {
     // phase1
     if (! something wrong) {
      //phase2
     }
    }

    But what if in phase 2 above there may be another exceptional condition? and another one.... you get something like

    do {
     // phase1
     if (! something wrong) {
      //phase2
      if (! something else wrong) {
       //phase 3    if (! something else again is wrong) {
        //phase 4
       }
      }
     }
    }

    Well, now compare it with the one you get with break:

    do {
     // phase1
     if (something wrong) break
     //phase2
     if (something else wrong) break
     //phase 3
     if (something else again is wrong) break
     //phase 4
    }

    Where is the WTF? This use of break is not a disguised goto but simply a poor man exception mechanism. The code above is fairly standard in Perl.

    By the way, the linux kernel make a eavy use of goto for error handling more or less in the way break is used in the above example. This allows better streamlining of the code by the CPU and, incidently avoid code falling out from the right margin, seing as Linus insists on 8 spaces tabs and 80 coulms lines.



  • [quote user="BitTwiddler"]

    There's a bunch of places where they want to do a goto, so instead they have a do/while(false) and then just "break" out of the middle - it's exactly like a goto, except that you can't easily find where you'll end up (some blocks are thousands of lines long, and the indentation is all fscked up), and if you're at the end, you can't easily find all the places that could send you there

    [/quote]

    Using break to jump around the code is not automatically bad - unlike goto, it's impossible to produce spaghetti code like this, because you can only jump in one direction (spaghetti code is what happens when you're jumping into and out of a block so much that if you draw a flowchart, it looks like a tangled mass of spaghetti).

    A good editor can show you where a break jumps to.

    The only real mess here is that they have blocks which are thousands of lines long - it doesn't matter what coding constructs you use there, the structure is going to be impossible to follow properly. Should have broken it up into subfunctions.
     



  • Okay, Leo, I'm almost convinced that it's a sane way to fake try/catch now :P. Even better ... ?

    err_t error = ERR_NO_ERROR;
    do {
     // phase1
     if (something wrong) { error.code = ERR_1; error.msg = "something"; break; }
     //phase2
     if (something else wrong) { error.code = ERR_2; error.msg = "something else"; break; }
     //phase 3
     if (something else again is wrong) { error.code = ERR_3; error.msg = "something else again"; break; }
     //phase 4
    }
    // pretend catch
    if(error.code == ERR_1){   // catch(Err1Exception error)
    } else if(error.code) { // catch(Exception error)
    } 

    I hope I never again have to write in a language without decent error handling (preferably heirarchical exceptions), but I do now see how this might be sane. As long as the breaks are clearly faked throws, though (comments! or assignment to an error struct to be 'thrown').



  • it may look awkward, but it's a pretty standard construct. deal.  its benefit over a goto is that it can be used several times in the same scope without having to readjust the labels and is copy/paste-safe.

    as for exceptions, they are a lot more expensive with stack unwinding and looking for handlers and what-not (though I'll agree that unless the code's in a tight loop, it's not much of an argument anymore).  but really, using exceptions for regular error handling is a faux pas.

    obvious variations on the theme:

    do
    {
        [...]
        if(_tfopen_s(&fo, moo, _T("wbT")) != 0)
        {
            SetLastError(cantAccessTempDir);
            break;
        }
        [...]
    }
    while(false);

    or something that I've seen crop up in win32 code:

        ok = ok && NULL != (dc = GetDC(NULL));
        ok = ok && NULL != (hdib = (HBITMAP)LoadImage(0, name, IMAGE_BITMAP, 0, 0, LR_CREATEDIBSECTION | LR_LOADFROMFILE));
    
    BITMAPINFO bi = {0};
    BITMAPINFOHEADER &amp;bih = bi.bmiHeader;
    bih.biSize = sizeof(bih);
    
    ok = ok &amp;&amp; 0 != GetDIBits(dc, hdib, 0, 0, NULL, &amp;bi, DIB_RGB_COLORS);
    if(ok)
    {
         [...]
    }
    // do this scope&#39;s releases</pre><p>looks weird if you&#39;ve never seen it before, but it&#39;s also pretty standard (`ok` can be an HRESULT in code that works with COM, etc.).</p>


  • A construct I've used several times is:

    if (!AllocationOne()) goto cleanup1;

    if (!AllocationTwo()) goto cleanup2;

    if (!AllocationThree()) goto cleanup3;

    <etc>

    <do real work>

    DeAllocationThree();

    cleanup3:

    DeAllocationTwo();

    cleanup2:

    DeAllocationOne();

    cleanup1:

     

    This lets me not only recover from whatever fails, but also set error codes to tell the user what went wrong.  The only other thing I use goto for is to temporarily bypass sections of code.

     

     



  • [quote user="sheriffpony"]as for exceptions, they are a lot more expensive with stack unwinding and looking for handlers and what-not (though I'll agree that unless the code's in a tight loop, it's not much of an argument anymore).  but really, using exceptions for regular error handling is a faux pas.[/quote]

    I don't understand this argument. I mean, I know exceptions are 'expensive' (i.e. slow), but surely the point is that they happen in, uh, exceptional conditions when the main path of your code can't execute, and thus speed isn't the issue, cleaning up correctly is?
     


Log in to reply