65

Currently I am working on a project where goto statements are heavely used. The main purpose of goto statements is to have one cleanup section in a routine rather than multiple return statements. Like below:

BOOL foo()
{
   BOOL bRetVal = FALSE;
   int *p = NULL;

   p = new int;
   if (p == NULL)
   {
     cout<<" OOM \n";
     goto Exit;
   }

   // Lot of code...

Exit:
   if(p)
   {
     delete p;
     p = NULL;
   }
   return bRetVal;
}

This makes it much easier as we can track our clean up code at one section in code, that is, after the Exit label.

However, I have read many places it's bad practice to have goto statements.

Currently I am reading the Code Complete book, and it says that we need to use variables close to their declarations. If we use goto then we need to declare/initialize all variables before first use of goto otherwise the compiler will give errors that initialization of xx variable is skipped by the goto statement.

Which way is right?


From Scott's comment:

It looks like using goto to jump from one section to another is bad as it makes the code hard to read and understand.

But if we use goto just to go forward and to one label then it should be fine(?).

11
  • 18
    My thoughts: Make up your mind about which language you're using. C++ has better tools available to guarantee clenaup (RAII, as mentioned below), but C doesn't, and in C, using goto in the manner you describe is very common.
    – jalf
    Dec 18, 2008 at 21:39
  • 2
    Goto makes the code unsafe to use in the presence of Exceptions. If you use RIAA it will do the cleanup for you and is exception safe. Dec 18, 2008 at 21:54
  • 1
    Also Note the test (p == NULL) will NEVER fail. 'p' is always guranteed to be a valid pointer after new completes. If new fails it will throw an exception (at which time the rest of your code will leak). Dec 18, 2008 at 21:58
  • 14
    @Martin York: The RIAA won't do your cleanup, only sue you. ;-) Dec 18, 2008 at 23:44
  • 7
    I pray to dog everyday to fix that ;-) Dec 19, 2008 at 6:14

35 Answers 35

1
2
0

That code has a bunch of problems, most of which were pointed out already, for example:

  • The function is too long; refactoring out some code into separate functions might help.

  • Using pointers when normal instances will probably work just fine.

  • Not taking advantage of STL types such as auto_ptr

  • Incorrectly checking for errors, and not catching exceptions. (I would argue that checking for OOM is pointless on the vast majority of platforms, since if you run out of memory you have bigger problems than your software can fix, unless you are writing the OS itself)

I have never needed a goto, and I've always found that using goto is a symptom of a bigger set of problems. Your case appears to be no exception.

0

Alien01 wrote: Currently I am working on a project where goto statements are heavily used. The main purpose of goto statements is to have one cleanup section in routine rather than multiple return statements.

In other words, you want to separate the program logic from simple repetitive tedious routines, like freeing a resource which might be reserved in different locations of code.

The exception handling technique is an error handling logic which works in parallel with program logic. It's a more elegant solution since it provides such separation while offering the ability to move control to other blocks of code exactly as the goto statement does, so I modified your script to look like this:

class auxNullPtrException : public std::exception {
    public:
        auxNullPtrException::auxNullPtrException()
            : std::exception( " OOM \n") {}
    };

    BOOL foo()
    {
        BOOL bRetVal = FALSE;
        try {
            int *p = NULL;
            p = new int;
            if (p == NULL)
            {
                throw auxNullPtrException();
            }
            // Lot of code...
         }
         catch(auxNullPtrException & _auxNullPtrException)
         {
             std::cerr<<_auxNullPtrException.what();
             if(p)
             {
                 delete p;
                 p = NULL;
             }
         }
         return bRetVal;
    }
0

Try it this way:

BOOL foo()
{
   BOOL bRetVal = FALSE;
   int *p = NULL;

   p = new int;
   if (p == NULL)
   {
     cout<<" OOM \n";
   }
   else
   {
       // Lot of code...
   }

   if (p)
   {
     delete p;
     p = NULL;
   }
   return bRetVal;
}

In the section "Lot of code", "Lot of code" is a good indication that you probably should refactor this section into one or more methods or functions.

7
  • 1
    This suffers from the same problems that have already been pointed out. Don't write code like that in C++. Not even low-level code. Dec 18, 2008 at 21:00
  • @Konrad: I understand you take issue with the various tests on p, but I think that's a side issue. Matthew's suggestion is AFAIK the classic "correct" method for avoiding GOTO -- use real branching logic instead.
    – Dave Costa
    Dec 18, 2008 at 21:04
  • 2
    Konrad is right, this misses a fundamental point. Screw the tests on p, they're not important. What matters is that you're using simple, error-prone branching when C++ gives you much better tools for handling cleanup, such as RAII.
    – jalf
    Dec 18, 2008 at 21:44
  • 1
    There's nothing you can do to prevent it, but you can usually fix it, too. In any case, it's something to seriously look at doing, before you start throwing in GOTOs and cleanup code. Dec 19, 2008 at 2:51
  • 1
    As far as I see it, Mattew's solution is a good C solution (remplace new/delete by malloc/free). But this is a BAD C++ solution: No RAII (either stack or smart pointer), ignorance the fact a failing new is supposed to throw, etc.. Re-reading the question I see both use of "new" and tag "c++"...
    – paercebal
    Dec 20, 2008 at 16:38
-1

From all the previous comments:

  1. goto is very very bad
  2. It makes code hard to read and understand.
  3. It can cause the well-known issue "spaghetti code"
  4. Everyone agree that it should not be done.

But I am using in following scenario

  1. It's used to go forward and only to one label.
  2. goto section is used to cleanup code and set a return value. If I don't use goto then I need to create a class of every data type. Like I need to wrap int * into a class.
  3. It's being followed in the whole project.

I agree that it's bad, but still it's making things much easier if followed properly.

4
  • the int* already exists. It's called std::auto_ptr or boost::shared_ptr. But what you're saying is "it doesn't matter because the entire project already uses gotos", so... why did you ask in the first place? Once again, pick a language. C uses goto, C++ doesn't.
    – jalf
    Dec 18, 2008 at 21:48
  • std::auto_ptr<> is your friend. It is also Exception safe. None of your code is. Dec 18, 2008 at 22:00
  • Easier often does not equate to correct, unfortunately. Dec 18, 2008 at 22:24
  • Going forward means you can bypass the initialization for some symbol. Assuming the compiler lets you do this, something with a constructor would be in an undefined state. My guess is that the only good way to use goto is to exit scopes. You're coding C++, so just do it. Use RAII.
    – paercebal
    Dec 20, 2008 at 17:00
-4

I don't know where this thing about goto come from...

In a compiled language each and every conditional instructions (if, switch, for, while, etc.) resolve in "cmp" "jmp" or "j??" in machine code (jmp IS goto).

In fact, a really well optimised code is aware of best execution paths and MUST therefore use "goto"... It's even better to do a linear piece of code with gotos than methods and calls when the stack is not used ("call" = "push"+"jmp").

There's absolutely no good reason why you shouldn't use GOTO in C++: generated code is full of "jmp"s everywhere anyway.

It's only a problem in scripts (where it is often not available), because the destination of goto has not necessarily been interpreted when goto instruction is.

The initial argument against goto is that the code is harder to check.

That's absurd: Code is not made to be checked (or only ONCE): It's made to be executed, to be small and fast. An optimized piece of code must avoid redundancy (by reusing the same instructions) and avoid runtime checks (exceptions that are not related to hardware malfunctions should all be avoided at design time, NOT at runtime).

3
  • 1
    Modern code is made to be readable, predictable and maintainable. Goto is often (but not always) a sign that this code is not. But it is silly to assert it "is not made to be checked." Most software development these days does involve code reviews.
    – Phil Hord
    Sep 21, 2012 at 22:27
  • .. And sometimes, complicated algorithms, described in comments with steps will be FAR more readable with goto stepX Apr 22, 2013 at 0:54
  • 1
    Beware premature optimisation, let the compiler add the gotos, make the code maintainable, optimise when absolutely proven there is a need!
    – Troyseph
    Oct 20, 2015 at 14:02
1
2

Not the answer you're looking for? Browse other questions tagged or ask your own question.