Sign in to follow this  
Bismuth

[C/C++] Coding style dilemma

Recommended Posts

I have a self-inflicted dilemma about different coding styles.

Suppose you have a list of functions/API's that need to be called in a particular order, as each one depends on the previous to succeed. We're also going to have to cleanup the resources allocated by these functions after we're finished. However, should one function fail, the whole routine fails, but we still have to bail out and free the resources that were allocated so far. If a function fails it returns zero, and non-zero otherwise.

A typical example would be setting up an OpenGL 3.x context. In order to get a OpenGL 3.x context you first have to create a dummy OpenGL context, get some pointers, and then destroy it. This is, however, only an example, I am talking about general approach in this thread. Consider this function:

[code]bool SomeFunction() {
bool result = false;

int a = DoSomething();
if (a != 0) {
int b = DoSomethingElse();
if (b != 0) {
int c = DoYetAnotherThing();
if (c != 0) {

... etc ...

// Final code here
result = true;

... etc ...

// Because we know DoYetAnotherThing succeeded we have to free the
// resources regardless of what happens in the nested code.
CleanupYetAnotherThing(c);
} else {
printf("DoYetAnotherThing.\n"); // Optional error msg
}

// Because we know DoSomethingElse succeeded we have to free the
// resources regardless of what happens in the nested code.
CleanupSomethingElse(b);
} else {
printf("DoSomethingElse failed.\n"); // Optional error msg
}

// Because we know DoSomething succeeded we have to free the
// resources regardless of what happens in the nested code.
CleanupSomething(a);

} else {
printf("DoSomething failed.\n"); // Optional error msg
}
return result;
}[/code]

This function performs what it's supposed to beautifully. It's like a pyramid - you climb it on one side and descent on the other. Should you trip anywhere while climbing it, you get instantly teleported to the other end and start descending without reaching the top. Unfortunately all those nested if's make the pyramid rather large, and thus, code much less readable. So I was looking for an alternative coding style to do this. Here's something I came up with:

[code]bool SomeFunction() {
bool result = false;

int a = DoSomething();
if (a == 0) goto CleanupA;

int b = DoSomethingElse();
if (b == 0) goto CleanupB;

int c = DoYetAnotherThing();
if (c == 0) goto CleanupC;

... etc ...

// Final code here
result = true;

... etc ...

CleanupC:
CleanupYetAnotherThing(c);

CleanupB:
CleanupSomethingElse(b);

CleanupA:
CleanupSomething(a);

return result;
}[/code]

Okay, getting rid of those nested if's sure makes the code a lot more readable. But as any decent book on C++ will tell you do not use goto's in your code. I think I'll heed that advice. How about this:


[code]int a, b, c;

bool SomeFunction() {

a = DoSomething();
if (a == 0) return false;

b = DoSomethingElse();
if (b == 0) return false;

c = DoYetAnotherThing();
if (c == 0) return false;

... etc ...

// Final code here
return true;

}

void SomeFunctionCleanup() {
... etc ...

if (c != 0) CleanupYetAnotherThing(c);
if (b != 0) CleanupSomethingElse(b);
if (a != 0) CleanupSomething(a);
}[/code]

The problem here is obvious. Not only we have to split the routine in two functions, they both have to use global variables in order to share resources. I am not sure whether encapsulating this in a class would solve anything.


What style should I use?

Share this post


Link to post
Share on other sites
Change your clean up function to accept a single parameter then simply call the clean up function on your != 0 checks. That way you only have one function handling cleanup instead of 3 and you don't have global variables.

As for needing your method to act in a transactional manner (that is, everything in the function acts as a single thing and either completely succeeds or completely fails) you should split your functions and game state. Your "DoStuff" would perform actions on virtual copies rather than committed version of the objects so that way if something fubars in the DoStuff routine there is no chance for your game to go all wonky. Only if DoStuff returns a true would you actually apply the results to your committed game objects and do things like move around models, play sounds, etc. Something like this:

[code]
void HandleActions()
{
bool stuffResult = DoStuff();

if(stuffResult)
{
CommitStuff();
}
}

bool DoStuff()
{
}
[/code]
Instead of returning a simple bool you could return a custom state object that contains a IsSuccessful bool then you commit by passing this custom state object around that would contain a state data store of some sort.

So the preceding code would look like:

[code]
void HandleActions()
{
CustomState stuffState = DoStuff();

if(stuffState.IsSuccessful)
{
CommitStuff(stuffState.StateData);
}
}

CustomState DoStuff()
{
}
[/code]

Share this post


Link to post
Share on other sites
You forgot an option: RAII. Create an object for A, B and C that performs the initialization in the constructor and cleanup in the destructor. Then create an A object, B object and C object.

Share this post


Link to post
Share on other sites
[b]landlocked[/b]: I'm not sure I understand. I do have only one function handling cleanup... cleanup for multiple API calls. I can't edit the API calls since they're from windows DLL's.

[b]Serapth[/b]: I am not exactly opposed to them. I've just rarely ever used them.

[b]SiCrane[/b]: If I create the three objects A, B and C in that order, what guarante do I have that they will be destroyed in the reverse order when they go out of scope?

I should have probably posted this in my first post, here's the code I was talking about:

[code]
bool InitializeOpenGl(HINSTANCE hInstance) {
bool result = false;

WNDCLASSEX wc;
memset(&wc, 0, sizeof(WNDCLASSEX));

wc.cbSize = sizeof(WNDCLASSEX);
wc.style = CS_OWNDC;
wc.lpfnWndProc = DummyWndProc;
wc.hInstance = hInstance;
wc.hIcon = LoadIcon(NULL, IDI_APPLICATION);
wc.hCursor = LoadCursor(NULL, IDC_ARROW);
wc.lpszClassName = L"DummyClass";
wc.hIconSm = LoadIcon(NULL, IDI_WINLOGO);

ATOM aWndClass = RegisterClassEx(&wc);
if (aWndClass != 0) {

HWND hWnd = CreateWindowEx(
NULL,
L"DummyClass",
L"Dummy",
WS_OVERLAPPEDWINDOW | WS_DISABLED | WS_ICONIC,
0,
0,
320,
240,
NULL,
NULL,
hInstance,
NULL
);

if (hWnd != 0) {
printf("Dummy window hWnd: 0x%X\n", hWnd);

HDC hDC = GetDC(hWnd);
if (hDC != 0) {
printf("Dummy window hDC: 0x%X\n", hDC);

PIXELFORMATDESCRIPTOR pfd;
memset(&pfd, 0, sizeof(PIXELFORMATDESCRIPTOR));

pfd.nSize = sizeof(PIXELFORMATDESCRIPTOR);
pfd.nVersion = 1; // Version Number
pfd.dwFlags = PFD_DRAW_TO_WINDOW |
PFD_SUPPORT_OPENGL |
PFD_DOUBLEBUFFER;
pfd.iPixelType = PFD_TYPE_RGBA;
pfd.cColorBits = 32;
pfd.cDepthBits = 16;
pfd.iLayerType = PFD_MAIN_PLANE;

int nPixelFormat = ChoosePixelFormat(hDC, &pfd);
if (nPixelFormat != 0) {
printf("Pixel format for dummy window: %i\n", nPixelFormat);

BOOL bResult = SetPixelFormat(hDC, nPixelFormat, &pfd);
if (bResult != 0) {

HGLRC temp_context = wglCreateContext(hDC);
if (temp_context != 0) {
wglMakeCurrent(hDC, temp_context);

//glewExperimental=TRUE;
GLenum err = glewInit();
if (err == GLEW_OK) {
if (GLEW_VERSION_3_2) {

// Final code here
result = true;

int major = 0;
int minor = 0;
glGetIntegerv(GL_MAJOR_VERSION, &major);
glGetIntegerv(GL_MINOR_VERSION, &minor);
printf("OpenGl version: %i.%i\n", major, minor);


} else {
printf("OpenGL 3.2 Not supported!!!\n");
}

} else {
printf("Cannot initialize GLEW.\n");
printf("%s\n", glewGetErrorString(err));
}

// Cleanup the existing GL context
wglMakeCurrent(NULL,NULL);
wglDeleteContext(temp_context);

} else {
printf("Unable to create GL context\n");
}

} else {
printf("Unable to set pixel format for dummy window.\n");
return 0;
}

} else {
printf("Unable to get pixel format.\n");
}

// Release drawing context
ReleaseDC(hWnd, hDC);

} else {
printf("Cannot access the device context.\n");
}

// The window is created, we have to destroy it.
DestroyWindow(hWnd);

} else {
printf("Cannot create window\n");
}

// Since the window class has been registered by this point we have to unregister it.
UnregisterClass(L"DummyClass", hInstance);

} else {
printf("Cannot register the class.\n");
}

return result;
}[/code]

Share this post


Link to post
Share on other sites
[quote name='Bismuth' timestamp='1310415150' post='4833960']
[b]SiCrane[/b]: If I create the three objects A, B and C in that order, what guarante do I have that they will be destroyed in the reverse order when they go out of scope?[/quote]
Yes, as long as they are in the same scope they will be destroyed in reverse order of creation. If you have
[code]
A a;
{B b;}
C c;
[/code]
b will be destroyed before c is even created because b will go out of scope first (though c should still be destroyed before a when leaving their scope). See section 6.6 paragraph 2 of the current version of the C++ standard.

Share this post


Link to post
Share on other sites
For the record, there isn't really a major problem with the code you posted. Most of those error conditions are show stoppers anyways ( if they occur, you ain't going to continue ) and it's not really all that hard to read. It's just the nature of initialization code sometimes to be a giant branching group of if statements.



Well, except your tab spacing is bloody gigantic. :D

Share this post


Link to post
Share on other sites
I would do something like this....

[code]
enum
{
DONE_NOTHING,
DO_SOMETHING,
DO_SOMETHING_ELSE,
DO_YET_ANOTHER_THING
};

static uint s_uInit = DONE_NOTHING;

void CleanUp(void)
{
/* clean up resources, fall through to clean up earlier stages */
switch (s_uInit)
{
case DO_YET_ANOTHER_THING:
CleanYetAnotherThing();
case DO_SOMETHING_ELSE:
CleanDoSomeThingElse();
case DO_SOMETHING:
CleanDoSomeThing();
}
}

bool Init(void)
{
if (DoSomething() && DoSomethingElse() && YetDoSomethingElse())
return true;

CleanUp();
return false;
}
[/code]

There is nothing wrong with your original style, the most important thing is that your consistent throughout your project.

- Kevin.

Share this post


Link to post
Share on other sites
I've had similar problems in the past, I went with the same approach as SiCrane. Its nice to be able to just have "return false" and not have to have any cleanup being called implicitly. You also get rid of all the nested ifs. One problem I see is keeping these things alive after the function has ended successfully (since your objects will want to cleanup when they are destroyed) but I'm sure you could deal with that (a "release" function or something).

Share this post


Link to post
Share on other sites
I'd use a mixture of RAII and, in the case of the OpenGL code, reverse the conditions of the 'if' checks to check for failures and bail rather than checking for success.

Basically, write your function as though the code will always succeed but check for failure and act on it.

[source]
bool InitializeOpenGl(HINSTANCE hInstance)
{
bool result = false;
WNDCLASSEX wc;
memset(&wc, 0, sizeof(WNDCLASSEX));
wc.cbSize = sizeof(WNDCLASSEX);
wc.style = CS_OWNDC;
wc.lpfnWndProc = DummyWndProc;
wc.hInstance = hInstance;
wc.hIcon = LoadIcon(NULL, IDI_APPLICATION);
wc.hCursor = LoadCursor(NULL, IDC_ARROW)
; wc.lpszClassName = L"DummyClass";
wc.hIconSm = LoadIcon(NULL, IDI_WINLOGO);
ATOM aWndClass = RegisterClassEx(&wc);
if (aWndClass == 0)
{
printf("Cannot register the class.\n");
return false;
}
HWND hWnd = CreateWindowEx(NULL,
L"DummyClass",
L"Dummy",
WS_OVERLAPPEDWINDOW | WS_DISABLED | WS_ICONIC,
0,0,320,240,NULL,NULL, hInstance, NULL);
if (hWnd == 0)
{
printf("Cannot create window\n");
return false;
}

printf("Dummy window hWnd: 0x%X\n", hWnd);
HDC hDC = GetDC(hWnd);
if (hDC == 0)
{
printf("Cannot access the device context.\n");
return false;
}

printf("Dummy window hDC: 0x%X\n", hDC);
PIXELFORMATDESCRIPTOR pfd;
memset(&pfd, 0, sizeof(PIXELFORMATDESCRIPTOR));
pfd.nSize = sizeof(PIXELFORMATDESCRIPTOR);
pfd.nVersion = 1; // Version Number
pfd.dwFlags = PFD_DRAW_TO_WINDOW | PFD_SUPPORT_OPENGL | PFD_DOUBLEBUFFER;
pfd.iPixelType = PFD_TYPE_RGBA;
pfd.cColorBits = 32;
pfd.cDepthBits = 16;
pfd.iLayerType = PFD_MAIN_PLANE;
int nPixelFormat = ChoosePixelFormat(hDC, &pfd);
if (nPixelFormat == 0)
{
printf("Unable to get pixel format.\n");
return false;
}

printf("Pixel format for dummy window: %i\n", nPixelFormat);
BOOL bResult = SetPixelFormat(hDC, nPixelFormat, &pfd);
if (bResult == 0)
{
printf("Unable to set pixel format for dummy window.\n");
return false;
}

HGLRC temp_context = wglCreateContext(hDC);
if (temp_context == 0)
{
printf("Unable to create GL context\n");
return false;
}

wglMakeCurrent(hDC, temp_context);
//glewExperimental=TRUE;
GLenum err = glewInit();
if (err != GLEW_OK)
{
printf("Cannot initialize GLEW.\n");
printf("%s\n", glewGetErrorString(err));
// Cleanup the existing GL context
wglMakeCurrent(NULL,NULL);
wglDeleteContext(temp_context);
return false;
}
if (!GLEW_VERSION_3_2)
{
printf("OpenGL 3.2 Not supported!!!\n");
return false;
}
// Final code here
result = true;
int major = 0;
int minor = 0;
glGetIntegerv(GL_MAJOR_VERSION, &major);
glGetIntegerv(GL_MINOR_VERSION, &minor);
printf("OpenGl version: %i.%i\n", major,minor);

// Release drawing context
ReleaseDC(hWnd, hDC);
// The window is created, we have to destroy it.
DestroyWindow(hWnd);
// Since the window class has been registered by this point we have to unregister it.
UnregisterClass(L"DummyClass", hInstance);

return true;
}
[/source]


Note how much simpler the code is to read without all the nested 'if' statements causing you to tab across the screen AND hide the error handling code towards the end of the function.

RAII would let you perform automagic clean up of resources allocated above (as the code as it stands doesn't remove alot of things), and exceptions or proper return codes would let you signal to higher up functions what went wrong (I'm assuming the 'printf' error codes here are for example and not real code...)

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this