Pattern to avoid "if failed cleanup" repetition

Question

I have some code that looks like this:

int myfunc()
{
     blah a;
     blah2 b;
     blah3 c;
     blah4 d;
     blah5 e;

     int iRes = DoSomething1(a, b, c);
     if (iRes > 0)
     {
         clean1(a, b, c);
         clean2(d, e);
         log_error();
         return 1;
     }

     iRes = DoSomething2(a, c, e);
     if (iRes > 0)
     {
         clean1(a, b, c);
         clean2(d, e);
         log_error();
         return 1;
     }

     ...

     iRes = DoSomething10(c, d, e);
     if (iRes > 0)
     {
         clean1(a, b, c);
         clean2(d, e);
         log_error();
         return 1;
     }

     clean1(a, b, c);
     clean2(d, e);
     return 0;
}

How, in C or C++, avoid the repetition of if (iRes > 0) { clean1(a, b, c); clean2(d, e); log_error(); return 1; } after each function calls?

Notes:

In the real code, these functions DoSomethingx() and cleanx() are API functions, not written by myself
I'd like to avoid having a second function clean() defined outside of myfunct() that would handle the cleanup + error
I thought about using preprocessor macro, but I doubt it's a good practice for such situations

Example:

This code is an example of such a situation: every 10 lines of code is in fact = 2 lines only for actually doing something + 8 lines of error testing and cleanup... Can we do nicer?

Are you using C or C++? Answers will be very different depending on which you are using — Justin, Jul 21 '17 at 23:45
@Justin I'm curious about both in fact. In this context, do you think it will change much? — Basj, Jul 21 '17 at 23:47
@Basj They will be very different. In C++, you would use RAII. C doesn't have RAII — Justin, Jul 21 '17 at 23:47
In the C++ case, if the error isn't too common, one would throw and catch an exception. This would gather most if not all of the clean-up. — user4581301, Jul 21 '17 at 23:47
@DavidBowling because it wouldn't really change much the general look. It would be : `iRes = DoSomething(...); if (iRes > 0) { cleanup_and_log(a, b, c, d, e); return 1; }` which is finally the same pattern than original code. — Basj, Jul 21 '17 at 23:49
@Basj Basically, you'd make the `DoSomethingN` throw an exception instead of mark the error via return values. If `DoSomethingN` is out of your control, because it's an external library, you'd wrap it to throw an exception. — Justin, Jul 21 '17 at 23:51
Oh I see @Justin. Then maybe a pure C solution would be interesting, as well as a C++ solution using RAII? — Basj, Jul 21 '17 at 23:51
@Basj Those would have to be two separate questions. As it is, having it tagged as both [c++] and [c] makes it too broad — Justin, Jul 21 '17 at 23:51
@Justin, Wouldn't wrapping `DoSomethingN` to throw exceptions be finally longer code and more complex than the original one? (It depends on how it would look like, I don't know for know) — Basj, Jul 21 '17 at 23:53
@Basj Depends. If you are only using the `DoSomethingN` functions in this one place, then *maybe* you won't want to wrap them. However, wrapping them makes the code elsewhere much easier to understand. Even a very small amount of wrapping can quickly pay off. Furthermore, popular C libraries usually have C++ wrappers so you don't have to wrap them yourself. — Justin, Jul 21 '17 at 23:54
@Justin Would it make sense to nearly copy/paste this question set to [c] only into another one for [c++]? (I'm okay about this, but not sure if it makes sense) — Basj, Jul 21 '17 at 23:55
Depends. If you don't use `iRes` for anything, you could just `if (DoSomething10(c, d, e)) throw myException();` — user4581301, Jul 21 '17 at 23:55
@Justin I will use them just once, they are winapi functions. (I'm using VC++ 2013). Don't know if they have "wrapped" versions? All the functions I'm speaking about [are here](https://msdn.microsoft.com/en-us/library/aa381911(v=VS.85).aspx). — Basj, Jul 21 '17 at 23:56
@user4581301 Interested for an answer with this! (with definition of myException() where? inside or outside myfunc?) — Basj, Jul 21 '17 at 23:57
Probably better that I send you to an existing document: https://isocpp.org/wiki/faq/exceptions — user4581301, Jul 21 '17 at 23:59
@user4581301 would it be `try { if (DoSomething10(c, d, e)) throw std::runtime_error(...) } catch(const std::runtime_error& e) { }` ? — Basj, Jul 22 '17 at 00:01
Right idea. The general problem with `std::runtime_error` vs your own exception is you might not be the only person throwing it. You could catch and mishandle a different `std::runtime_error` and wind up in a world of hurt. In this case, Windows system calls aren't throwing anything, so you probably have a pass. Keep an eye out if you have another library thrown in there. — user4581301, Jul 22 '17 at 00:09
@user4581301 Ok. Following your initial advice, how would you modify [this](https://pastebin.com/RknsfUZu) to make it close to what you had in mind? — Basj, Jul 22 '17 at 00:13
@Justin would it really be *a big deal* to have just one additional answer in [c++]? It would be similar to thousands of questions "How to do this with JS or jQuery" which are **both** high-upvoted and not closed as too broad nor locked. Does it really make sense to copy/paste this question with [c++] to find the C++ way to do it? — Basj, Jul 22 '17 at 10:03
@Basj jQuery is a JS library. C++ and C are two very different languages. Ask this as another question in the other tag (or look for a duplicate). It's not bad at all to ask another question. Currently, it's a lot like asking, "what's the fastest way to get to [location] on car or bike?" That's multiple questions, which is too broad for stackoverflow — Justin, Jul 22 '17 at 15:22

score 3 · Accepted Answer · answered Jul 22 '17 at 00:00

As Justin mentioned, the answers will be quite different depending on the language.

If you are in C, this is one of the final places where goto has a stronghold in the language. For example, if you have:

int myfunc()
{
     blah a;
     blah2 b;
     blah3 c;
     blah4 d;
     blah5 e;

     int iRes = DoSomething1(a, b, c);
     if (iRes > 0)
     {
         goto error_cleanup;
     }

     iRes = DoSomething2(a, c, e);
     if (iRes > 0)
     {
         goto error_cleanup;
     }

     /*...*/

     iRes = DoSomething10(c, d, e);
     if (iRes > 0)
     {
         goto error_cleanup;
     }

     /* SUCCESS */
     clean1(a, b, c);
     clean2(d, e);
     return 0;

 /* ERROR EXIT POINT */
 error_cleanup:

     clean1(a, b, c);
     clean2(d, e);
     log_error();
     return 1;
 }

However, in C++ we need to deal with this cleanup code even when exceptions are thrown, which puts another wrench into the scheme of things. However, in c++ we also have RAII, which means destructors is the way to go.

Thanks! I thought quickly about Basic's `GOTO 10` before, but I didn't know this existed / was still in use in C! — Basj, Jul 22 '17 at 00:03
Related good answer: https://stackoverflow.com/a/245761/1422096 — Basj, Jul 22 '17 at 08:57

Lee Daniel Crocker · Answer 2 · 2017-07-24T19:23:18.303

3

Here's a trick I like that avoids the goto:

bool success = false;

do {
    r = do_something();
    if (r) break;

    r = do_something_else();
    if (r) break;

    r = do_something_again();
    if (r) break;

    success = true;
} while(0);

if (! success) {
    clean_up();
}

edited Jul 24 '17 at 19:23

answered Jul 22 '17 at 00:49

Lee Daniel Crocker

12,927
3
29
55

You have a typo in your success initialization. Ps i like this better than goto because you know what's happening locally and don't have to scroll down to see that the goto is in the right place. – wawiesel Jul 22 '17 at 01:35
The assignment is 'wrong' because of the `-`, but that doesn't actually affect the operation of the code on most machines since `-0` is still `0`. – Jonathan Leffler Jul 22 '17 at 07:32

score 0 · Answer 3 · answered Jul 22 '17 at 07:42

For the given code, you could use any of a number of variations on the them of:

int myfunc()
{
     blah a;
     blah2 b;
     blah3 c;
     blah4 d;
     blah5 e;

     int iRes;
     if ((iRes = DoSomething1(a, b, c)) > 0 ||
         (iRes = DoSomething2(a, c, e)) > 0 ||
         ...
         (iRes = DoSomething10(c, d, e)) > 0)
     {
         clean1(a, b, c);
         clean2(d, e);
         log_error();
         return 1;
     }

     clean1(a, b, c);
     clean2(d, e);
     return 0;
}

If it doesn't matter whether log_error() is called before or after clean1() and clean2(), you can use:

int myfunc()
{
     blah a;
     blah2 b;
     blah3 c;
     blah4 d;
     blah5 e;

     int iRes;
     if ((iRes = DoSomething1(a, b, c)) > 0 ||
         (iRes = DoSomething2(a, c, e)) > 0 ||
         ...
         (iRes = DoSomething10(c, d, e)) > 0)
     {
         log_error();
     }

     clean1(a, b, c);
     clean2(d, e);
     return 0;
}

Or even:

int myfunc()
{
     blah a;
     blah2 b;
     blah3 c;
     blah4 d;
     blah5 e;

     int iRes;
     if ((iRes = DoSomething1(a, b, c)) > 0 ||
         (iRes = DoSomething2(a, c, e)) > 0 ||
         ...
         (iRes = DoSomething10(c, d, e)) > 0)
     {
         // Don't need to do anything here
     }

     clean1(a, b, c);
     clean2(d, e);
     if (iRes > 0)
         log_error();
     return 0;
}

And the body of the if might be the final assignment:

int myfunc()
{
     blah a;
     blah2 b;
     blah3 c;
     blah4 d;
     blah5 e;

     int iRes;
     if ((iRes = DoSomething1(a, b, c)) > 0 ||
         (iRes = DoSomething2(a, c, e)) > 0 ||
         ...
        )
     {
         iRes = DoSomething10(c, d, e);
     }

     clean1(a, b, c);
     clean2(d, e);
     if (iRes > 0)
         log_error();
     return 0;
}

This refactoring depends on the clean-up being identical in all cases. Usually, it isn't quite that systematic. Since you don't show any of a, b, c, d or e being initialized, it's hard to be sure what's really safe. If the doSomethingN() functions are in C++ and take arguments by reference, then doSomething1() could initialize a, b, c — but the call to clean2() could occur before d or e have values.

Thank you for your answer. It think this would require all the work to be done in one single `if ((DoSomething1(a, b, c) > 0) || (DoSomething2(a, c, e) > 0) || ... || (DoSomething10(c, d, e) > 0))` which is often impossible (see the example https://msdn.microsoft.com/en-us/library/aa381911(v=VS.85).aspx). — Basj, Jul 22 '17 at 08:59

Pattern to avoid "if failed cleanup" repetition

3 Answers3

Linked