What is proper way to deal with a function that has multiple return paths?

[Red Squirrel]

Say I have this code in a function:

int Function()
{

int error = SomeInitFunction();
if(error)
{
return error;
}

//allocate some memory or something

error = SomeOtherFunction();
if(error)
{
return error;
}

error = SomeOtherFunction2();
if(error)
{
return error;
}
//(etc)

//finalize everything, deallocate memory etc

return 0;
}

There are multiple return paths, but just exiting like that means I may not be freeing up memory or finishing anything that may need to be done, depending on what the function does. What is the best way to deal with this? I've seen simply making sure to deallocate stuff at each return point, but that seems error prone to me.

Is the better way to simply have lot of nested ifs and have a single return path? Ex instead of just having the if(error) you then have an else { } and put the rest of the code in there. Is that considered better practice?

I've actually seen people use goto in situations like this, but I don't think that's considered a practical thing either.

 

[ru!ner]

Unless you're using something like C's malloc, either the object going out of scope or the garbage collector should free things up for you. You didn't say what language you're talking about.

 

[Red Squirrel]

Oh forgot to mention, C++, using 3rd party libs, which potentially could be using new/malloc internally. There are functions you need to call when you are done with the library. (freetype)

 

[ru!ner]

Do you need to re-initialize it every time that function runs or can you decouple the init from the function and just free it once on program exit?

Otherwise I'd just use a #define CLEANUP_FREETYPE macro (or function if you have enough cleanup to do) and call it before every return. The compiler is smart enough to inline small functions if its too big for a define, and it definitely looks like you'll cause a leak if you don't clean up properly.

 

[Quartz-1]

There are multiple return paths, but just exiting like that means I may not be freeing up memory or finishing anything that may need to be done, depending on what the function does. What is the best way to deal with this?

There's a better way.

See all those segments which say if (error) {return error} ? What you should be doing is something like if not (error) {continue to do stuff}.

Basically you only do stuff when there's no error. Then the code always falls through to the clean-up code at the end.

 

[ru!ner]

There's a better way.

See all those segments which say if (error) {return error} ? What you should be doing is something like if not (error) {continue to do stuff}.

Basically you only do stuff when there's no error. Then the code always falls through to the clean-up code at the end.

Unless you have to check for errors multiple times like he is showing, then you end up with code that's indented off the side of the screen. The other way creates much more readable code. Readability > all.

 

[devman]

Unless you have to check for errors multiple times like he is showing, then you end up with code that's indented off the side of the screen. The other way creates much more readable code. Readability > all.

Code with many returns can be just as problematic as code that is highly nested. Also I read Quartz-1's comment as preferring to use error as a sentenial value for the same flat structure with a return at the end instead of returning immediately, not necessarily to nest the structure.

The real issue is high cyclomatic complexity. If a function has too many decisions being made it should be broken up in to smaller, more manageable and readable pieces.

Keep cyclomatic complexity low and structure flow control for readability.

 

[ru!ner]

Yeah you're right, I read Quartz's comment wrong. That would be better than a number of returns.

 

[sharknice]

Say I have this code in a function:

int Function()
{

int error = SomeInitFunction();
if(error)
{
return error;
}

//allocate some memory or something

error = SomeOtherFunction();
if(error)
{
return error;
}

error = SomeOtherFunction2();
if(error)
{
return error;
}
//(etc)

//finalize everything, deallocate memory etc

return 0;
}

There are multiple return paths, but just exiting like that means I may not be freeing up memory or finishing anything that may need to be done, depending on what the function does. What is the best way to deal with this? I've seen simply making sure to deallocate stuff at each return point, but that seems error prone to me.

Is the better way to simply have lot of nested ifs and have a single return path? Ex instead of just having the if(error) you then have an else { } and put the rest of the code in there. Is that considered better practice?

I've actually seen people use goto in situations like this, but I don't think that's considered a practical thing either.

You can put the cleanup part in a function and call it whenever you need it.
ex.

void cleanupStuff() {
//finalize everything, deallocate memory etc
}

if(error) {
cleanupStuff();
return error;
}

 

[aL Mac]

I once saw code where a developer used do... while (false) loops haha.. I wouldn't suggest this:

do
{
  int error = doSomething();

  if(error)
    break;

  error = doSomethingElse();
  
  if(error)
    break;

  // do some work
} while(false)

// Clean up

I think at this point you might as well use a goto

Anyway, Quartz's suggestion seemed reasonable

 

[Red Squirrel]

Never thought of doing not, that would work too, though still end up with nested stuff but it will be cleaner than doing it with elses. I probably could break some stuff into small private functions too though then have a Cleanup function. But think I'll do the if not way, that seems like the easiest. I don't have that many error checks so it should not nest too badly.

 

[Sgraffite]

Can you make each function that is doing something part of a class with a destructor that cleans itself up when it goes out of scope? I haven't done C++ since college so this is potentially a horrible idea.

 

[Red Squirrel]

Suppose that is an option though this already is a class so it would be adding a lot of extra complexity to it. Basically it's a png class and this one function is to write a string of text, so it uses freetype. The class itself does not keep track of anything specific to freetype as it's all initialized and deinitialized within that single function call.

I still need to do memory leak tests as well so I'll have to force errors to see how it reacts as it is now.

 

[Dogs]

C++ has exceptions, so why aren't you using exceptions? Writing if statements to check for error values on every call that can fail makes your code hideously unreadable and difficult to maintain, even if care is taken to try to make the error logic as clean as possible.

If your functions are written to use exceptions for exceptional situations, and you move your cleanup code into a private cleanup function, you can rewrite your code as follows:

void Function() {
    try {
        SomeInitFunction();
        SomeOtherFunction();
        SomeOtherFunction2();
        CleanupFunction();
    } catch(int e) {
        CleanupFunction();
        throw e;
    }
}

...or, if you're interfacing with code higher up that doesn't use exceptions:

int Function() {
    int result = 0;
    try {
        SomeInitFunction();
        SomeOtherFunction();
        SomeOtherFunction2();
    } catch(int e) {
        result = e;
    }
    CleanupFunction();
    return result;
}

In either case, you'll get much nicer code than if you wrote all of the error-handling boilerplate yourself using conditional logic.

Note: '3rd party libraries aren't using exceptions' isn't a very good reason, since you can wrap those calls and throw an exception if they don't return a successful result.

 

[Red Squirrel]

Wouldn't that mean having to do a try/catch statement for every single time I want to call that function? Would make the front end code quite ugly. The idea behind this class is so things can be done very elegantly with very simple code. In this case it's a png class with text functions. This is a sample program used to test it:

int main()
{	
	AppLogger logger("debug");  //logger stuff optional
	logger.ConsoleOut=true;
	logger.AutoFlush=true;
	logger.DebugLevel=4;  

	rspng testimg("freetypetest.png");  
	testimg.SetAppLogger(&logger);	
	
	testimg.SetSize(1500,1200);
	
	
	testimg.PlotRect(10,0,100, 1000,255,0,0);
	testimg.PlotRect(110,0,200, 1000,0,255,0);
	testimg.PlotRect(210,0,300, 1000,0,0,255);
	
	
	rspng img2("png.png");
	img2.ReadFromFile();	
	testimg.WriteRspng(50,200, img2,40, true);
    
	
	for(int i=1;i<=20;i++)
	{	
	  testimg.WriteString(10, 10 + i*50, "this is a test string!","Impact.ttf",8+(i-1)*2, 0, 0, 0,256-(i*4));
	  testimg.WritePixel(testimg.GetTextLastX(),testimg.GetTextLastY(),255,0,0,255);
	}
	testimg.mod_flattenalpha=false;
	
	for(int i=1;i<=20;i++)
	{	
	  testimg.WriteString(400, 10 + i*50, "this is a test string!","Courier_New.ttf",8+(i-1)*2, 0, 0, 0,256-(i*4));
	  testimg.WritePixel(testimg.GetTextLastX(),testimg.GetTextLastY(),255,0,0,255);
	}
	
	testimg.WriteToFile();
return 0;
}

Very nice and clean and simple to read/generate/edit png images.

Even if any of those things fail it's non fatal so the app can continue to execute. With exceptions, it would crash if I don't capture the exception. At least that's how I understand it. Never really worked much with exceptions so maybe there's something I'm missing. I've just witnessed some classes that use them, and I ended up having to write my own wrapper around those classes so I'm not stuck having to do exception checking throughout my whole program, was kind of a pain.

 

[Dogs]

Wouldn't that mean having to do a try/catch statement for every single time I want to call that function? Would make the front end code quite ugly. The idea behind this class is so things can be done very elegantly with very simple code. In this case it's a png class with text functions.

As I showed in the second code block, if the code calling that code really can't tolerate being re-written to handle exceptions, you can use exceptions internally while still returning an error code externally.

...but in your case, I think you'll find exceptions to be easy enough to use in your test program.

int main() {
        int result = 0;
        try {	
	AppLogger logger("debug");  //logger stuff optional
	logger.ConsoleOut=true;
	logger.AutoFlush=true;
	logger.DebugLevel=4;  

	rspng testimg("freetypetest.png");  
	testimg.SetAppLogger(&logger);	
	
	testimg.SetSize(1500,1200);
	
	
	testimg.PlotRect(10,0,100, 1000,255,0,0);
	testimg.PlotRect(110,0,200, 1000,0,255,0);
	testimg.PlotRect(210,0,300, 1000,0,0,255);
	
	
	rspng img2("png.png");
	img2.ReadFromFile();	
	testimg.WriteRspng(50,200, img2,40, true);
    
	
	for(int i=1;i<=20;i++)
	{	
	  testimg.WriteString(10, 10 + i*50, "this is a test string!","Impact.ttf",8+(i-1)*2, 0, 0, 0,256-(i*4));
	  testimg.WritePixel(testimg.GetTextLastX(),testimg.GetTextLastY(),255,0,0,255);
	}
	testimg.mod_flattenalpha=false;
	
	for(int i=1;i<=20;i++)
	{	
	  testimg.WriteString(400, 10 + i*50, "this is a test string!","Courier_New.ttf",8+(i-1)*2, 0, 0, 0,256-(i*4));
	  testimg.WritePixel(testimg.GetTextLastX(),testimg.GetTextLastY(),255,0,0,255);
	}
	
	testimg.WriteToFile();
        } catch (int e) {
                result = e;
        }
        return result;
}

If your code is structured well, the exceptions shouldn't make your code messier or more difficult to work with.

Even if any of those things fail it's non fatal so the app can continue to execute. With exceptions, it would crash if I don't capture the exception.

That is correct...The code I showed above would result in halt-on-error behaviour, but in general halt-on-error is what you want. If you don't care if your functions succeed or not, do you really need them to be called in the first place? Letting the functions fail silently is generally an undesirable behaviour because the program can look as though it executed successfully, when it hasn't. Your users have a hard time figuring out whether the program completed everything it was supposed to, or whether the result they got back was only partially complete. That results in a lot of overhead involved in simply trying to use the application and make sure it did what you want, so it's a design style that should be carefully evaluated before using. You have to very carefully decide if having code that fails quietly and has the program continue in a possibly unpredictable manner is really a good idea.

But even if you do decide that 'fire-and-forget', failsilent code is the right way to go, there's an easy fix for that. Don't rethrow the exceptions, as I showed in my previous post:

...or, if you're interfacing with code higher up that doesn't use exceptions:

int Function() {
    int result = 0;
    try {
        SomeInitFunction();
        SomeOtherFunction();
        SomeOtherFunction2();
    } catch(int e) {
        result = e;
    }
    CleanupFunction();
    return result;
}

You'll still get the readability of the exception handling in your internal code, and thanks to encapsulation, the code calling that code doesn't need to know or care that you're using exception handling internally.

Never really worked much with exceptions so maybe there's something I'm missing. I've just witnessed some classes that use them, and I ended up having to write my own wrapper around those classes so I'm not stuck having to do exception checking throughout my whole program, was kind of a pain.

They're a vital tool, and I strongly recommend you consider learning how to work with them. You'll struggle to be able to write large, high-quality systems without them.

Also, you should consider why the code you wrapped was throwing exceptions. Unless the code throwing the exceptions is very poorly constructed, exceptions are generally something you want to be catching and handling. Eating and ignoring exceptions is an anti-pattern.
http://c2.com/cgi/wiki?SweepItUnderTheRugAntiPattern

 

[wonderfield]

If you're checking for exceptions "throughout your entire program", you probably aren't leveraging the fact that exceptions propagate through the call stack and can be caught (and potentially dealt with) at higher levels. Just because a function may throw an exception doesn't mean you have to wrap that function's call in a try/catch: you can probably deal with that scenario, in whatever way makes sense for your application, further up the stack, and probably should. Hell, depending on user expectations for your app's long-term stability and ability to recover from exceptional circumstances, that might just mean a single try/catch in main(). The more resilience you need to maintain, it logically follows that the extent of the exception handling you'll want to do will increase, but many applications don't need to be particularly resilient.

 

[Wiseguy2001]

A few things:-

1.Don't be scared of exception's they needed crash the whole thing (you should be capturing the exception and display a cleaner error message anyway). I've just don't a search on the current project I'm working on. It's ~3000 lines of code, of that I'm throwing 223 exceptions, with 22 try blocks. These aren't used for logic, it's when a state is discovered (usually an input) when it makes no sense to continue the current task/ thead, so GTFO.

As others have said throwing exceptions when things go pear shapped, actually leads to far more stability.

2.There's times when you may not wish to throw exceptions, but try plan B. and C... Here you have your if statements, to try different things. For instance trying to detect what formate the input, is it forms? is it json? is it xml? Then if it isn't any of those things then throw an exception to stop trying to process the request.

Also called a variable error is confusion, most would expect this to be an exception, or at least a boolean, then treating the integer as a boolean in the if statements makes thing even stranger.


3.The original question, should you exit early? Here's two snippets (they have the same outputs). I'll let you weigh up the pro's and cons.

bool MyContainsA(string value) {
    bool found = false;
    for (int i = 0; i < _list.length; i++) {
        if (_list[i] == value)
            found = true;
    }
    return found;
}

bool MyContainsB(string value) {
    for (int i = 0; i < _list.length; i++) {
        if (_list[i] == value)
            return true;
    }
    return false;
}

 

[Red Squirrel]

Oh wait, that's right so the catch will well, catch the exception and prevent a crash, but it will serve as a quick way to stop execution of some code so cleanup can be made after, right?

Now in Dog's example:

int Function() {
    int result = 0;
    try {
        SomeInitFunction();
        SomeOtherFunction();
        SomeOtherFunction2();
    } catch(int e) {
        result = e;
    }
    CleanupFunction();
    return result;
}

How does the exception system know if those functions failed? They don't throw exceptions. Usually functions return a non 0 value, so does it go by that? What if you're dealing with a function that returns 0 on failure? (ex: a Boolean that returns false)


As I progressed through this function I realized there was not that many steps to check for error so ended up just doing it the else/if way after all. I still have more testing to do with this but this is the final function:

int rspng::WriteString(int x, int y, string text,string fontfile,int size=1, int red=0, int green=0, int blue=0, int alpha=255, bool downwards=false)
	{
	  #ifdef rslib_png_freetype
	  /*
	  Thanks to Freetype.org for example code.  Lot of this code derrived from it.   
	  */
	  
	  //init lib and font object:
	  FT_Library  ftlibrary;
	  FT_Face     face;	  
	  
	  //init freetype library 
	  int error = FT_Init_FreeType( &ftlibrary );
	  if ( error )
	  {
	    Debug(1,"Error " + Int2Str(error) + " initializing freetype @ FT_Init_FreeType()");
	  }
	  else
	  {
	      //open font file:	
	      error = FT_New_Face( ftlibrary, fontfile.c_str(), 0, &face );
	      if ( error == FT_Err_Unknown_File_Format )
	      {
	        Debug(1,"Error " + Int2Str(error) + " reading font file (file found) @ FT_New_Face()");
	      }
	      else if ( error )
	      {
	        Debug(1,"Error " + Int2Str(error) + " reading font file (file not found) @ FT_New_Face()");
	      }
	      else
	      {
	          
	          //set font size
	          error = FT_Set_Pixel_Sizes(face,size,0);
	          if(error)
	          {
	            Debug(1,"Error " + Int2Str(error) + " setting size @ FT_Set_Pixel_Sizes()");
	          }
	          else
	          {
	              
	              FT_GlyphSlot  slot = face->glyph;  /* a small shortcut */
	              FT_UInt       glyph_index;
	              int           n;
	              
	              int pen_x = x;
	              int pen_y = y;
	              
	              for (n = 0; n < text.length(); n++ )
	              {
	                    /* load glyph image into the slot (erase previous one) */
	                    error = FT_Load_Char( face, text[n], FT_LOAD_RENDER );
	                    if ( error ) continue;  /* ignore errors */
	                  
                        //loop to draw each pixel of char:

                        FT_Bitmap*  bitmap = &slot->bitmap;
                        int px = pen_x + slot->bitmap_left;
                        int py = pen_y - slot->bitmap_top;

                        FT_Int  i, j, p, q;
                        FT_Int  x_max = px + bitmap->width;
                        FT_Int  y_max = py + bitmap->rows;

	                    for ( i = px, p = 0; i < x_max; i++, p++ )
	                    {
	                      for ( j = py, q = 0; j < y_max; j++, q++ )
	                      {
                            if ( i < 0 || j < 0 || i >= m_width || j >= m_height ) continue;

                            unsigned char pixel = bitmap->buffer[q * bitmap->width + p];

                            if(pixel>0)
                            {
                                //the font data uses alpha to make it smooth, we don't want this to actually use png alpha
                                //so we blend it in.  However if alpha was specified we still want to allow it to work.
                                //so we first do a flatten mode alpha, and if function's alpha is not 255 then we simply apply the new alpha. 

                                if(!mod_flattenalpha)
                                {
                                    mod_flattenalpha=true;//turn flatten on

                                    WritePixel(i, j ,red,green,blue,pixel);

                                    if(alpha<255)
                                    {
                                        pngpixel * tmppixel = GetPixel(i,j);
                                        tmppixel->SetAlpha(alpha);
                                    }

                                    mod_flattenalpha=false; //turn it back off (know it was off due to if statement we are in)
                                }
                                else //not flattening alpha, so we'll just do our own averaging and use real alpha layer
                                {
                                    WritePixel(i, j ,red,green,blue,(pixel+alpha)/2);
                                }		                        
                            }
	                      }
	                    }
	                    
	                    m_lasttextendposx=i;
	                    m_lasttextendposy=j;

	                  /* increment pen position */
	                  
	                  if(downwards)
	                  {
	                    pen_y += size;
	                  }
	                  else
	                  {	                  
	                    pen_x += slot->advance.x >> 6;
	                  }
	            }	
	         }
	      }
	    }
	
	  FT_Done_Face    ( face );
	  FT_Done_FreeType( ftlibrary );
	 
	  	  
	  return error; //return error
	  #endif
	  
	  //TODO: hard coded basic text function for systems that may have trouble with compiling with freetype
	  
	  Debug(1,"Error: text not supported, freetype not compiled in.  Add #define rslib_png_freetype to enable.  Type freetype-config to confirm that it is installed on system.");
	  return -1;
	}

 

[Dogs]

How does the exception system know if those functions failed? They don't throw exceptions. Usually functions return a non 0 value, so does it go by that? What if you're dealing with a function that returns 0 on failure? (ex: a Boolean that returns false)

If the functions don't throw exceptions when they fail, wrap them. In your wrapper, check to see if they completed successfully, and if they don't, throw the error code as an exception. It's the best you can do with code that doesn't take advantage of exceptions...

 

[Red Squirrel]

Oh I see, so I'd still do the if statements, but then I just throw an exception (perhaps an ID to identify which function that failed) instead of returning. That would force the code execution to continue in the catch block, then I can clean up at the end of the try/catch and have a common function exit point. Makes sense now. I need to read up some more on exceptions and experiment with that but think I pretty much understand now.