🔒 Locked

Code design - function failures

Started by Exellion Jul 14, 2003 at 8:37 AM 14 replies 2k views

Original Post

Jul 14, 2003 at 8:37 AM

Hi all. I have a little code design problem. I know several ways to improve the code as it is, using either extra functions or goto, but Im not satisfied with any of them. Oh, well, lets take a look at my problem first. I have a lot of functions that work this way so this is just a generic exemple:

HRESULT MyFunction()
{
    //init

    char * string = new char[20];    

    //call a function. clean and return error code if failure

    if (FAILED(SomeFunction(string)))
    {
        delete string;
        return (ERROR_CODE);
    }

    //call another function. clean and return error code if failure

    if (FAILED(SomeOtherFunction(string)))
    {
        delete string;
        return (ERROR_CODE);
    }
    
    //and so on with more functions..



    //clean and return some success code

    delete string;
    return (SUCCESS_CODE);
}

Now, as you can see, the code gets ugly because of all the cleaning inside every if-failed-statement. When I have even more cleaning stuff to do, like 5 lines or something, this layout forces me to write lots of extra code and it looks ugly. So, as I said before, I know several ways to improve the code and here they are: 1. Create a function called MyFunctionCleanup which performs all the cleaning and call it right before every return. In the above exemple this would require just as much code in the primary function but when the cleanup consists of more code this helps a bit. 2. Using goto. This solution looks great, but everyone tells you not to use goto and I personally never do. Should I? 3. Doing something like the following:

HRESULT MyFunctionCore(char * string)
{
    if (FAILED(SomeFunction(string)))
        return (ERROR_CODE);
    if (FAILED(SomeOtherFunction(string)))
        return (ERROR_CODE);
    return (SUCCESS_CODE);
}

HRESULT MyFunction()
{
    //Init

    char * string = new char[20];
    
    //Main function call

    bool resultat = MyFunctionCore(string));
   
    //Cleanup

    delete string;
    return (resultat);
}

If you gotten this far, thanks for taking some of your valuable time and trying help me. Im not necessarily looking for the perfect, supergeneric, extravagant solution when posting here, but rather some opinions on which of my three ways you would choose (and why) or some new thoughts or methods of doing it. I simple want to know what you think and/or how you deal with situations like this one. Thanks!

Greg K

Jul 14, 2003 at 8:46 AM

If you find that you are getting a lot of repeat code look at what is getting repeated and put that in a function. In this case it would be your cleanup code. I think if you put your cleanup code into a function and then just call cleanup and return then that is the best you can do without losing code readability.

-Greg

Reverie Entertainment

antareus

Jul 14, 2003 at 8:47 AM

Throw an exception.

Wrap resources in wrapper classes such as std::auto_ptr that release them at the end.

Exellion

Author

Exellion

Jul 14, 2003 at 8:53 AM

AP: Yeah that''s something I''ve tried as well, but in lots of situations you want the function to return right away. Since data may be invalid after a failed function call it could cause an access violation or something like that if you continue.

Antareus: Never used std::auto_ptr before, but I''ll take a look at it right away.

Everyone else: Keep it coming

krez

156

krez

Jul 14, 2003 at 8:59 AM

declare the "string" variable on the stack, so it cleans itself up.

i.e. "char string[20];"
instead of "char * string = new char[20];"

davepermen

160

davepermen

Jul 14, 2003 at 9:00 AM

with smart pointer


HRESULT MyFunction()
{
    boost::scoped_ptr<char> string(new char[20]);

    if (FAILED(SomeFunction(string.get()))) // not sure about the get syntax anymore, sorry..

        return (ERROR_CODE);

    if (FAILED(SomeOtherFunction(string.get())))
        return (ERROR_CODE);
    return (SUCCESS_CODE);
}

with exceptions and smart poitner


void MyFunction() {
   boost::scoped_ptr<char> string(new char[20]);
   SomeFunction(string.get());
   SomeOtherFunction(string.get());
}

with about all c++ capabilities:


void MyFunction() {
   std::string str;
   SomeFunction(str);
   SomeOtherFunction(str);
}

the last two, of course, only if the SomeFunction and SomeOtherFunction are recodeable as well to use exceptions, or std::string&

else, the first one is about the approach you want..

if you add exceptions, you can create a


void ThrowOnError(LRESULT lresult) {
 switch(lresult) {
   case SOME_ERROR: throw std::exception("some error!");
   ....
 }
}

and you could setup your func then like this:


void MyFunction()
{
    boost::scoped_ptr<char> string(new char[20]);

    ThrowOnError(SomeFunction(string.get());
    ThrowOnError(SomeOtherFunction(string.get());
}

it all just depends on what you have to interfer with..

but i definitely suggest using exceptions in your code.. they make your code as readable as before and still make it save..

oh, and... RAII is a keyword for you, too..

"take a look around" - limp bizkit
www.google.com

Jedyte

Jul 14, 2003 at 9:21 AM

quote:
Original post by Exellion
AP: Yeah that''s something I''ve tried as well, but in lots of situations you want the function to return right away. Since data may be invalid after a failed function call it could cause an access violation or something like that if you continue.

Then just simply use if..else instead of if.

Psybr

Jul 14, 2003 at 9:22 AM

What''s wrong with using goto for something like this? Actually, why is goto a no-no in the first place?

Waverider

108

Waverider

Jul 14, 2003 at 9:31 AM

goto = spaghetti omg debugging nightmare code.

When it gets out of hand.

Depend on it once, you keep depending on it, make logic structures out of it = plan on rewriting your code instead of modifying it. Avoid it!

[edited by - Waverider on July 14, 2003 10:32:51 AM]

MENTAL

Jul 14, 2003 at 9:32 AM

"goto" is evil. not even evil in a good way either, but in the "i''m so evil i don''t need to convince anyone how evil i am" kinda way. you shouldn''t even speak it''s name without the risk of envoking the wrath of your peers.

Stoffel

Jul 14, 2003 at 10:50 AM

I''ve seen device driver reference code that uses goto in this exact way. If you don''t have the ability to use exceptions (the DDK doesn''t support them), it''s proper to use goto. I think that''s just about the only time it is, though.

Jan Wassenberg

105

Jan Wassenberg

Jul 14, 2003 at 1:30 PM

N00bs are told "don''t ever use goto" so that they don''t output spaghetti code. People who know what they''re doing recognize goto is often useful.
Yes, you can avoid it by extracting code into functions and returning (but wait! only one exit per block! one!!1), or with flags. goto is still in the language because these alternatives are sometimes much worse.

consider a simplified example:


insert_into_list
  if(!mem allocated)
    goto alloc
  if(already in list)
    goto skip_insert

  if(bucket full)
  {
    if(too many buckets allocated)
      abort insert, i.e. return

alloc:
    ...
    update end of list
  }
  else
    add to end of bucket

  init new element

skip_insert:
  update element

This code is tightly coupled, so splitting it up into several functions would really mess things up.
It''s also bad enough without a bunch of nested ifs(). With some comments about what''s going on, it''s quite readable.

Getting off topic. goto is not always bad. In the original case, I see no problem with goto cleanup or whatever. auto ptrs are fine for simple cases, but don''t cover everything.

swiftcoder

Mod

2,296

swiftcoder

Jul 14, 2003 at 1:42 PM

Most serious programmers use goto a lot, but only for error handling as in your question, i.e.

bool MyFunction() {
if (!Function1())
goto error;
if (!Function2())
goto error;

return true;

error:
// Cleanup
return false;
}

this gives very readable code, and has significantly less overhead than smart-pointers or exception-handlers.

---------------------------------------------------------------------------
Making cross-platform software actually cross-platform (meaning compiles on OS X)

[edited by - swiftcoder on July 14, 2003 2:43:37 PM]

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.