What is the correct way to clean up code?

Question

When you have several lines of codes that initiate other objects, is there a cleaner way to clean up the objects than what is shown below? I initiate several objects in one function and check to see if they fail- but I have a bunch of redundant code that I have to keep typing. Is what shown below the correct method of doing this? Or are there cleaner ways? I am aware of the do { } while(false) method and the goto method- but they are not clean and feel messy.

    if( bind(s, (sockaddr *)&saddr, sizeof(sockaddr)) == SOCKET_ERROR ) {
        printf("bind() failed.\n");
        closesocket(s);
        CloseHandle(g_hIOCompletionPort);
        CloseHandle(g_hShutdownEvent);
        WSACleanup();
    }

    if( listen(s, 60) == SOCKET_ERROR ) {
        printf("listen() failed.\n");
        closesocket(s);
        CloseHandle(g_hIOCompletionPort);
        CloseHandle(g_hShutdownEvent);
        WSACleanup();
    }

    g_hAcceptEvent = WSACreateEvent();
    if( g_hAcceptEvent == WSA_INVALID_EVENT ) {
        printf("WSACreateEvent() failed.\n");
        closesocket(s);
        CloseHandle(g_hIOCompletionPort);
        CloseHandle(g_hShutdownEvent);
        WSACleanup();
    }

This is off topic here; try http://codereview.stackexchange.com. :) — Almo, Mar 21 '14 at 14:20
You appear to be calling `listen` even in the case where `bind` failed and you've done the cleanup. Which is odd. — Steve Jessop, Mar 21 '14 at 14:46

score 3 · Answer 1 · answered Mar 21 '14 at 14:24

If you're using C++ then RAII (Resource Acquisition Is Initialization) is the preferred way. Basically you acquire or attach a resource to the class (typically during construction) and free it up during destruction.

For example, in your code you'd have some sort of Handle class which held onto the g_hIOCompletionPort handle and called CloseHandle in its destructor.

jxh · Accepted Answer · 2014-03-21T15:37:24.833

Most often, RAII would be used in C++, in which the code is organized with objects that have constructors and destructors, and the cleanup is performed in the destructor. Thus, destruction of the object is sufficient, which limits the amount of replicated code.

class Server {
    SOCKET s;
    HANDLE iocp;
    HANDLE shutdown;
    std::string err_str;
public:
    ~Server () {
        if (!err_str.epmty()) std::cerr << err_str << '\n';
        closesocket(s);
        CloseHandle(iocp);
        CloseHandle(shutdown);
        WSACleanup();
    }
    //...
};

In addition to RAII, C++ also provides exception handling, which could also be made to work in your case. The try block would have the socket code. When the socket code could throw an exception on error, and the catch block could take care of the clean up.

try {
    if (bind(...) == SOCKET_ERROR) {
        throw ...something...;
    }
    if (listen(...) == SOCKET_ERROR) {
        throw ...something...;
    }
    ...
}
catch (...something...) {
    ...cleanup code;
}

In C, there is no RAII equivalent. Nor are there exceptions. However, exception handling can be emulated with setjmp() and longjmp(), as is done with cexcept. Although there is no direct support for RAII, there is nothing preventing you from organizing your C code into objects with associated clean up functions.

struct Server {
    SOCKET s;
    HANDLE iocp;
    HANDLE shutdown;
    const char *err_str;
};

void destroy_server (Server *server) {
    /* ... */
}

If you have a lot of code that follows a sequence, but any of them might need to do the cleanup, you can organize the code in a state machine like fashion.

enum { STATE_INIT, STATE_SUCCESS, STATE_ERROR, STATE_STOP } state = STATE_INIT;

while (state != STATE_STOP) {
    switch (state) {
    case STATE_INIT:    state = do_server_init(); break;
    case STATE_SUCCESS: state = do_server(); break;
    case STATE_ERROR:   state = do_server_cleanup(); break;
    case STATE_STOP:    break;
    default:            fprintf(stderr, "unexpected state: %d\n", state);
                        state = STATE_ERROR;
    }
}

score 0 · Answer 3 · edited May 23 '17 at 11:57

0

In C++ RAII is a very common pattern for such a case.

For plain C: if I write goto I will be killed, so I don't. But look for example here: Valid use of goto for error management in C?

edited May 23 '17 at 11:57

Community

1
1

answered Mar 21 '14 at 14:21

Danvil

22,240
19
65
88

What is the correct way to clean up code?

3 Answers3