Using goto in c++ code

[JVene]

What occurs to me is that it seems obvious, without any further explication on the matter, that releasing memory does not happen unless an allocation is performed.

This is C++, right?

Here's another approach plan

int SomeFunction(int parameter)
{
 if ( Initiate() )
     {   
       foo1();
       shared_ptr<t> p( AllocateMemory() );
        
       if ( p )
          {
           foo2();

           if ( RunAnalysis() ) 
               {
                foo3();
                return true;
               }
          } 

     }

 return false;
}

Or if your coding standard is strict regarding only one return place

int SomeFunction(int parameter)
{
 bool rval = false;
 
 if ( Initiate() )
     {   
       foo1();
       shared_ptr<t> p( AllocateMemory() );
        
       if ( p )
          {
           foo2();

           if ( RunAnalysis() ) 
               {
                foo3();
                rval = true;
               }
          } 

     }

 return rval;
}

And it seems reasonable that 3 indentations is at or approaching that "enough" limit, plus the rest may likely be better inside type t:

int SomeFunction(int parameter)
{
 if ( Initiate() )
     {   
       foo1();
       shared_ptr<t> p( AllocateMemory() );
        
       if ( p )
          {
            return p->Process( parameter );
          } 

     }

 return false;
}




bool t::Processing( int parameter )
{
 bool rval = false;
   
 foo2();

 if ( RunAnalysis() ) 
     {
       foo3();
       rval = true;
     }

 return rval;
}

In this latter case, foo2 and foo3 may well be part of class t, as they are only performed if things are going well, appear 'part of' what's happening with regard to t, at least after initializations occurred.


Now riddle me this:

What happens to allocated memory in the context of exceptions thrown by foo3, foo2 or RunAnalysis in the "goto" versions above?

And, what are your options about that?

..think about it...

[plong]

I'm a strict one-entry-one-exit kind of guy, so the one-return snippet looks good to me. Extra returns are second cousins to gotos.

This is what I'd like to see:

int SomeFunction(int parameter)
{
	int retVal = FALSE;

	if (Initiate()) {
		if (AllocateMemory()) {
			if (RunAnalysis()) {
				retVal = TRUE;
			}
		} else {
			ReleaseMemory();
		}
	}
	
	return retVal;
}

or the variable-less but cringe-worthy:

int SomeFunction(int parameter)
{
	return Initiate() && (AllocateMemory() ? RunAnalysis() : ReleaseMemory(), FALSE);
}

I've always heard that 4 or 5 levels was the limit for nesting control structures, but a quick Google shows "3" and "3 to 5," so whatever.

[JVene]

Torvalds made a somewhat famous quote (or at least it's attributed to him), "If you have more than 3 indentations in your code, you need to fix your code".

I'm personally less convinced of the similarity between multiple returns and goto. I would agree it was more of an issue prior to C++ and without strict use of RAII, and I recall dealing with bugs associated with multiple returns in "the old days". The point of caution against it does have merit.

[JohnW@Wessex]

Torvalds made a somewhat famous quote (or at least it's attributed to him), "If you have more than 3 indentations in your code, you need to fix your code".

I'd be cautious about assuming that someone is a coding style guru, merely because they wrote a popular piece of software.

[binyo66]

I am agree with Kheun.
Lets compare this 2 functions, and lets assuming the caller
needs to get the exact returns value (0 no errors). As you
may know sometimes GetLastError function is not clear enough.

1)
long RunLnkFromPIDL(long pShExecInfo /*in out */)
{
HRESULT hres;
IShellLink* psl;
long lRet=0;
WORD *wsz=0;
SHELLEXECUTEINFO *pInfo=(SHELLEXECUTEINFO *)pShExecInfo;
if (!pInfo)
return -1;
pInfo->lpVerb = NULL;
// Get a pointer to the IShellLink interface.
hres = CoCreateInstance(CLSID_ShellLink, NULL,
CLSCTX_INPROC_SERVER, IID_IShellLink, (void **)&psl);
if (SUCCEEDED(hres)) {
IPersistFile* ppf;

// Get a pointer to the IPersistFile interface.
hres = psl->QueryInterface(IID_IPersistFile,
(void **)&ppf);
if (SUCCEEDED(hres)) {
wsz=new word [MAX_PATH];
if (wsz)
{ // failed to allocated
// Ensure that the string is Unicode.
lRet=MultiByteToWideChar(CP_ACP, 0, pInfo->lpFile, -1, wsz,
MAX_PATH);
if (SUCCEEDED(lRet)) {
// Load the shortcut.
hres = ppf->Load(wsz, STGM_READ);
if (SUCCEEDED(hres)) {

// Resolve the link.
assert(pInfo->hwnd);
hres = psl->Resolve(pInfo->hwnd, SLR_ANY_MATCH);
if (SUCCEEDED(hres)) {
//LPITEMIDLIST pidl=(LPITEMIDLIST *)*((long *)pidlAddr);
hres = psl->GetIDList((_ITEMIDLIST **)&(pInfo->lpIDList));
ShellExecuteEx(pInfo);

//GetModuleFileName(pInfo->hInstApp, sName, MAX_PATH);
}
else
lRet=-7;
}
else
lRet=-6;
}
else
lRet=-5
delete [] wsz;
}
else
lRet=-4;
// Release the pointer to the IPersistFile interface.
ppf->Release();
}
else
lRet=-3;
// Release the pointer to the IShellLink interface.
psl->Release();
}
else
return -2;

if (wsz)
delete [] wsz;
return lRet;
}

2)
long RunLnkFromPIDL(long pShExecInfo /*in out */)
{
HRESULT hres;
IShellLink* psl;
long lRet=0;
WORD *wsz=0;
IPersistFile* ppf;
SHELLEXECUTEINFO *pInfo=(SHELLEXECUTEINFO *)pShExecInfo;
if (!pInfo)
return -1;
pInfo->lpVerb = NULL;
// Get a pointer to the IShellLink interface.
hres = CoCreateInstance(CLSID_ShellLink, NULL,
CLSCTX_INPROC_SERVER, IID_IShellLink, (void **)&psl);
if (FAILED(hRes))
return -2;

// Get a pointer to the IPersistFile interface.
hres = psl->QueryInterface(IID_IPersistFile,
(void **)&ppf);
if (FAILED(hRes) {
lRet=-3;
goto END;
}
wsz=new word [MAX_PATH];
if (!wsz) {
lRet=-4;
goto END1;
}
// Ensure that the string is Unicode.
lRet=MultiByteToWideChar(CP_ACP, 0, pInfo->lpFile, -1, wsz,
MAX_PATH);
if (FAILED(lRet)) {
lRet=-5;
goto END1;
}
// Load the shortcut.
hres = ppf->Load(wsz, STGM_READ);
if (FAILED(hRes)) {
lRet=-6;
goto END1;
}
// Resolve the link.
assert(pInfo->hwnd);
hres = psl->Resolve(pInfo->hwnd, SLR_ANY_MATCH);
if (SUCCEEDED(hres)) {
//LPITEMIDLIST pidl=(LPITEMIDLIST *)*((long *)pidlAddr);
hres = psl->GetIDList((_ITEMIDLIST **)&(pInfo->lpIDList));
ShellExecuteEx(pInfo);

}
else
lRet=-7;
END1:
ppf->Release();
END:
psl->Release();
if (wsz)
delete [] wsz;
return lRet;
}

I think 2 is more readable. And, IMO, one the main purpose of structural programming
language is to make code more readable, so that it is easy to maintain. therefore, I am
still using goto to clean up local variables. BTW this is probably more C rather
than C++. And, sorry, I just paste from my code, and copy to notepad, it really looks ugly, all spaces gone . I havent post a lot, so I dont know how to make it space yet.

[Kheun]

I don' think I will ever use goto in C++ as there are multiple ways to avoid it and still writing clean code. However, it will be a different story when the programming language of choice is C. It is especially so for embedded software where the device memory footprint is small. Even though there had been many advances in C++ compiler development, the minimum memory footprint requirement for C++ is much larger than C.

[Arjay]

Using goto is okay in C++ if you can't figure out any other way to do it.

Though, I haven't happen to run across any cases where it was necessary, but I would kind of like to see a case where it was the only alternative.

[Paul McKenzie]

Using goto is okay in C++ if you can't figure out any other way to do it.

I don't necessarily agree.

If the coder isn't very good, then the excuse they will use for using goto is that they couldn't figure out a way. Of course, there is always a way around "goto".

Regards,

Paul McKenzie

[Arjay]

I don't necessarily agree.

If the coder isn't very good, then the excuse they will use for using goto is that they couldn't figure out a way. Of course, there is always a way around "goto".

Regards,

Paul McKenzie

That's my point. There's no need to use goto.