Flow Control: Exceptions versus if/then

[jeffrey@toad.net]

I hope I do not get a religious argument started, but here goes...

When I program, I try to write code that is easy to read, and easy to use. This should make it easy to maintain, and reuse by others (copy and paste).

The most generic way I know to control flow through a function is with goto statements (while keeping the function easy to read).

Consider the following:

LONG CAESEncRegKey::ReadData(BYTE **pcbData, DWORD *pdwSize) const
{
LONG lResult = ERROR_SUCCESS;
HKEY hKey = NULL;

ASSERT( NULL != pcbData && NULL != pdwSize );

// Sanity Check
if( NULL == pcbData || NULL == pdwSize ) {
lResult = ERROR_INVALID_PARAMETER;
goto FINISHED;
}

// Open the Key
lResult = RegOpenKeyEx( _hKey, _szSubKey, 0, KEY_READ, &hKey );

// Sanity Check
if( ERROR_SUCCESS != lResult ) { goto FINISHED; }

// Query for needed buffer size
lResult = RegQueryValueEx( hKey, _szValueName, 0, NULL, NULL, pdwSize );

if( ERROR_SUCCESS != lResult ) { goto FINISHED; }

// Allocate a buffer
*pcbData = new BYTE[ *pdwSize + sizeof(TCHAR) ];

// Sanity Check
if( NULL == *pcbData ) {
lResult = ERROR_NOT_ENOUGH_MEMORY;
goto FINISHED;
}

// Query for the actual value
lResult = RegQueryValueEx( hKey, _szValueName, 0, NULL, *pcbData, pdwSize );

FINISHED:

if( NULL != hKey ) { RegCloseKey( hKey ); }

return lResult;

}

If using it/then/else, the code could be nested four levels. I don’t think this is easy to read (again, my humble opinion).

Another option would be to use try/throw/catch. However, this option would force the program to use Exception handling, which is something a programmer may not want to do. For example, eVC 4.0 does not implement exception handling, so one could write a program/class/function that is basically useless.

With that said, the following usess Exceptions:

LONG CAESEncRegKey::ReadData(BYTE **pcbData, DWORD *pdwSize) const
{
LONG lResult = ERROR_SUCCESS;
HKEY hKey = NULL;

try {

// Sanity Check
if( NULL == pcbData || NULL == pdwSize ) {
throw (LONG) ERROR_INVALID_PARAMETER;
}

// Open the Key
lResult = RegOpenKeyEx( _hKey, _szSubKey, 0, KEY_READ, &hKey );

// Sanity Check
if( ERROR_SUCCESS != lResult ) {
throw (LONG) lResult;
}

// Query for needed buffer size
lResult = RegQueryValueEx( hKey, _szValueName, 0, NULL, NULL, pdwSize );

if( ERROR_SUCCESS != lResult ) {
throw (LONG) lResult;
}

// Allocate a buffer
*pcbData = new BYTE[ *pdwSize + sizeof(TCHAR) ];

// Sanity Check
if( NULL == *pcbData ) {
throw (LONG) ERROR_NOT_ENOUGH_MEMORY;
}

// Query for the actual value
lResult = RegQueryValueEx( hKey, _szValueName, 0, NULL, *pcbData, pdwSize );
}

catch ( LONG lr ) {

lResult = lr;
}

catch ( ... ) {

lResult = ERROR_GEN_FAILURE;
}

if( NULL != hKey ) { RegCloseKey( hKey ); }

return lResult;
}


I suppose my question is, what is the preferred method?


Jeff

[SuperKoko]

To avoid gotos, and exceptions, you can define an helper class for the registry key:
This class will open the key with RegOpenKeyEx, when constructed, and release it when destroyed (if it has been succesfully constructed).
If you don't want to use exceptions, you can just test if the object has been successfully constructed. If not you return an error code.

With that method, you can correctly throw exceptions, without needing to use a catch block!
For example that thing:

catch ( ... ) {

lResult = ERROR_GEN_FAILURE;
}

Is ugly, because it means that the exceptions which could be thrown by a subfunction (for example if your function calls a callback function), cannot launch exceptions correctly, because they will not be kept by the user.
You should at least rethrow such exception!

catch ( ... ) {

lResult = ERROR_GEN_FAILURE;
if( NULL != hKey ) { RegCloseKey( hKey ); }
throw;
}

But, it is highly preferable to use RAII.
In that case, there is no need of try/catch!

[SuperKoko]

Another thing : it is preferable to launch classes exceptions instead of builtins types exceptions.
For example, you can derive an exception registry_error from std::runtime_error, and use it instead of LONG, which may conflict with another function which throws another LONG exception!

[Paul McKenzie]

First, I wouldn't use exceptions for the purpose of saying that "a registry key doesn't exist". You use exceptions for exceptional cases. Not having a certain registry key IMO is not an exceptional error, unless that registry key is required for the OS to run correctly or something of that sort.

Second, if you are to use goto, then the target should always be down further in the code, never before the call to goto. Therefore (again IMO), your first form of goto is acceptable (in terms of usage or goto).

Third, you could avoid this by creating a "Registry" class and have that handle these issues of checking for exsitence of keys and opening them and handling the excetions there. If something fails, you set some failure flags in the class and check those -- somewhat similar to the C++ stream functions when you ask to open a file and the file doesn't exist or can't be opened for some reason, or the class can throw it's own exception if it can't be created because of those issues.

Fourth, the ANSI standard "new" does not return NULL if there is an error -- an exception is thrown (std::bad_alloc).

Edit: I see that SuperKoko beat me to it with the idea of creating a class to handle the Registry issues.

Regards,

Paul McKenzie