Code improvement suggestions

I'd just like some general advice on the code below. I wrote it in order to initialize a global configuration structure with values from the registry. It works fine, but I really wasn't very happy with how it turned out structure wise. Does anybody have any suggestions on how it could be improved in terms of formatting, optimising, reduncy reducing, ect.?

Code:

//Include Files//////////////////////////////////////////////////////////////////////////// #include <windows.h> #include "config.h" #include "debug.h" #include "global_definitions.h" #include "memory.h" #include "numio.h" #include "stringio.h" /////////////////////////////////////////////////////////////////////////////////////////// //Global variables///////////////////////////////////////////////////////////////////////// MYAPPSTATUS myappStatus; /////////////////////////////////////////////////////////////////////////////////////////// //Functions prototypes///////////////////////////////////////////////////////////////////// void InitMyappStatus(MYAPPSTATUS * myappStatus); LONG CheckRegValue(HKEY hKey, LPTSTR regValue, DWORD dataType, void * configPointer, void * defValue, DWORD defValueSize, DWORD maxValueSize, BOOL notFoundIsError, BOOL * errorMB); //This function cannot check for datatype missmatches /////////////////////////////////////////////////////////////////////////////////////////// //Function definitions///////////////////////////////////////////////////////////////////// void InitMyappStatus(MYAPPSTATUS * myappStatus) { DWORD keyDisposition; BOOL notFoundIsError; BOOL errorMB = 1; HKEY hKey; BYTE defaultVal[MAX_PATH]; //Default values are limited to the size of MAX_PATH to make my life easier and since they should never need to be longer than this anyway RegCreateKeyEx( HKEY_CURRENT_USER, myapp_DEFAULT_REGKEY, 0, (LPTSTR) "", REG_OPTION_NON_VOLATILE, KEY_READ | KEY_WRITE, (LPSECURITY_ATTRIBUTES) NULL, &hKey, &keyDisposition); notFoundIsError = (keyDisposition == REG_OPENED_EXISTING_KEY); * (BOOL *) &defaultVal = 0; CheckRegValue(hKey, "Test", REG_BINARY, (void *) &myappStatus->test, (void *) &defaultVal, sizeof(BOOL), sizeof(BOOL), notFoundIsError, &errorMB); if(myappStatus->test > 1 || myappStatus->test < 0) //Invalid value test { myappStatus->test = * (BOOL *) &defaultVal; ErrorMessage("Registry value \"Test\" was invalid. Default value used", NULL, NULL, MB_ICONEXCLAMATION, 1); } return; } LONG CheckRegValue(HKEY hKey, LPTSTR regValue, DWORD dataType, void * configPointer, void * defValue, DWORD defValueSize, DWORD maxValueSize, BOOL notFoundIsError, BOOL * errorMB) { char * mergedString[2]; char * numString; LONG error; if(error = RegQueryValueEx(hKey, regValue, NULL, NULL, (LPBYTE) configPointer, &maxValueSize)) { if(error == 2) //This is what's returned when the value is not found { if(notFoundIsError) { *errorMB = ( IDOK == ErrorMessage( (mergedString[0] = StringMerge("Failed to Read Registry Key:", regValue, "\0")) , (mergedString[1] = StringMerge("Couldn't find registry value: ", regValue, ". If you have not deleted any ", VERSION, " keys then you may have a corrupted registry. Press OK to continue or Cancel to continue and skip any subsequent registy errors.", "\0")) , NULL, MB_ICONEXCLAMATION | MB_OKCANCEL, *errorMB) ); free((void *) mergedString[0]); free((void *) mergedString[1]); } else { ErrorMessage((mergedString[0] = StringMerge("The following registry value was successfully initialized: ", regValue, "\0")), NULL, NULL, MB_ICONINFORMATION, 0);; free((void *) mergedString[0]); } } else { *errorMB = ( IDOK == ErrorMessage( (mergedString[0] = StringMerge("Failed to Read Registry Key:", regValue, "\0")), (mergedString[1] = StringMerge("Failed to read registry value \"", regValue, "\". Error code: ", numString = UINTToString((UINT) error, 10, 0), ". Press OK to continue or Cancel to continue and skip any subsequent registy errors.", "\0")), NULL, MB_ICONEXCLAMATION | MB_OKCANCEL, *errorMB) ); free((void *) mergedString[0]); free((void *) mergedString[1]); free((void *) numString); } RegSetValueEx(hKey, regValue, 0, dataType, (CONST BYTE *) defValue, defValueSize); memcpy(configPointer, defValue, defValueSize); } return error; } ///////////////////////////////////////////////////////////////////////////////////////////

>>Formatting
- Loose all the ////////. Personally I find it ugly, but maybe its just me...
- Stop with the 400+ characters per line. Your code shouldn't go any wider than say 100 characters.

Quote:

Originally posted by Hammer
>>Formatting
- Loose all the ////////. Personally I find it ugly, but maybe its just me...

Well personally I kinda like them since it helps make clear where one portion of the file ends and the other begins.

Quote:

Originally posted by Hammer
- Stop with the 400+ characters per line. Your code shouldn't go any wider than say 100 characters.

Point taken. But what would you say would be the best way to format a line containing so many functions, such as this one:

Code:

*errorMB = ( IDOK == ErrorMessage( (mergedString[0] = StringMerge("Failed to Read Registry Key:", regValue, "\0")), (mergedString[1] = StringMerge("Failed to read registry value \"", regValue, "\". Error code: ", numString = UINTToString((UINT) error, 10, 0), ". Press OK to continue or Cancel to continue and skip any subsequent registy errors.", "\0")), NULL, MB_ICONEXCLAMATION | MB_OKCANCEL, *errorMB) );

How about something like

Code:

/* *--------------------------------------------------------------------- * This is a sample *--------------------------------------------------------------------- */ mergedString[0] = StringMerge("Failed to Read Registry Key:", regValue, "\0"); mergedString[1] = StringMerge("Couldn't find registry value: ", regValue, ". If you have not deleted any ", VERSION, " keys then you may have a corrupted registry." " Press OK to continue or Cancel to continue and skip " " any subsequent registy errors.", "\0") *errorMB = (IDOK == ErrorMessage(mergedString[0], mergedString[1], NULL, MB_ICONEXCLAMATION | MB_OKCANCEL, *errorMB));

Ah ok, I suppose you could just stick those things on seporate lines. anyway though, regarding how I check for and invalid value, the code get's really redundant if you check for more than one thing, but I really couldn't think of a way to improve this a whole lot wihtout totally rething my code structure. Any suggestions on this?

Quote:

Originally posted by Xzyx987X
Ah ok, I suppose you could just stick those things on seporate lines. anyway though, regarding how I check for and invalid value, the code get's really redundant if you check for more than one thing, but I really couldn't think of a way to improve this a whole lot wihtout totally rething my code structure. Any suggestions on this?

Do what Hammer suggested and repost.

We can't get past 400+ characters a line to see what your code does. Very few are going to scroll right for 3 miles to read your code and any question you might have had.

Err... sorry. To be honest I'm kind of used to reading long lines of code so I don't usually consider it a problem. The cases with the stings were an exception to that of course. Anyway, here's the code edited so you can read it more easily:

Code:

//Include Files//////////////////////////////////////////////////////////////////////////// #include <windows.h> #include "config.h" #include "debug.h" #include "global_definitions.h" #include "memory.h" #include "numio.h" #include "stringio.h" /////////////////////////////////////////////////////////////////////////////////////////// //Global variables///////////////////////////////////////////////////////////////////////// MYAPPSTATUS myappStatus; /////////////////////////////////////////////////////////////////////////////////////////// //Functions prototypes///////////////////////////////////////////////////////////////////// void InitMyappStatus(MYAPPSTATUS * myappStatus); LONG CheckRegValue(HKEY hKey, LPTSTR regValue, DWORD dataType, void * configPointer, void * defValue, DWORD defValueSize, DWORD maxValueSize, BOOL notFoundIsError, BOOL * errorMB); //This function cannot check for datatype missmatches /////////////////////////////////////////////////////////////////////////////////////////// //Function definitions///////////////////////////////////////////////////////////////////// void InitMyappStatus(MYAPPSTATUS * myappStatus) { DWORD keyDisposition; BOOL notFoundIsError; BOOL errorMB = 1; HKEY hKey; BYTE defaultVal[MAX_PATH]; //Default values are limited to the size of MAX_PATH to make my life easier and since they should never need to be longer than this anyway RegCreateKeyEx( HKEY_CURRENT_USER, myapp_DEFAULT_REGKEY, 0, (LPTSTR) "", REG_OPTION_NON_VOLATILE, KEY_READ | KEY_WRITE, (LPSECURITY_ATTRIBUTES) NULL, &hKey, &keyDisposition); notFoundIsError = (keyDisposition == REG_OPENED_EXISTING_KEY); //The code below is what I think needs work. I don't think this much code should be required for a single registry value * (BOOL *) &defaultVal = 0; CheckRegValue(hKey,"Test", REG_BINARY, (void *) &myappStatus->test, (void *) &defaultVal, sizeof(BOOL), sizeof(BOOL), notFoundIsError, &errorMB); if(myappStatus->test > 1 || myappStatus->test < 0) //Invalid value test { myappStatus->test = * (BOOL *) &defaultVal; ErrorMessage("Registry value \"Test\" was invalid. Default value used", NULL, NULL, MB_ICONEXCLAMATION, 1); } return; } LONG CheckRegValue(HKEY hKey, LPTSTR regValue, DWORD dataType, void * configPointer, void * defValue, DWORD defValueSize, DWORD maxValueSize, BOOL notFoundIsError, BOOL * errorMB) { char * mergedString[2]; char * numString; LONG error; if(error = RegQueryValueEx(hKey, regValue, NULL, NULL, (LPBYTE) configPointer, &maxValueSize)) { if(error == 2) //This is what's returned when the value is not found { if(notFoundIsError) { mergedString[0] = StringMerge("Failed to Read Registry Key:", regValue, "\0"); mergedString[1] = StringMerge("Couldn't find registry value: ", regValue, ". If you have not deleted any ", VERSION, " keys then you may have a corrupted registry. Press OK to continue or Cancel to continue and skip any subsequent registy errors.", "\0"); *errorMB = (IDOK == ErrorMessage(mergedString[0], mergedString[1], NULL, MB_ICONEXCLAMATION | MB_OKCANCEL, *errorMB)); free((void *) mergedString[0]); free((void *) mergedString[1]); } else { mergedString[0] = StringMerge("The following registry value was successfully initialized: ", regValue, "\0"); ErrorMessage(mergedString[0], NULL, NULL, MB_ICONINFORMATION, 0);; free((void *) mergedString[0]); } } else { numString = UINTToString((UINT) error, 10, 0); mergedString[0] = StringMerge("Failed to Read Registry Key:", regValue, "\0"); mergedString[1] = StringMerge("Failed to read registry value \"", regValue, "\". Error code: ", numString, ". Press OK to continue or Cancel to continue and skip any subsequent registy errors.", "\0"); *errorMB = (IDOK == ErrorMessage(mergedString[0], mergedString[1], NULL, MB_ICONEXCLAMATION | MB_OKCANCEL, *errorMB)); free((void *) mergedString[0]); free((void *) mergedString[1]); free((void *) numString); } RegSetValueEx(hKey, regValue, 0, dataType, (CONST BYTE *) defValue, defValueSize); memcpy(configPointer, defValue, defValueSize); } return error; } ///////////////////////////////////////////////////////////////////////////////////////////

A little better, but 173 is still longer than necessary. Hammer did suggest 100. Try Preview next time, and if the bottom scroll bar is less than 75% of the window width shorten some more. And you don't need to indent 8 spaces. 4 is more of an accepted standard

Quote:

Originally posted by Xzyx987X
Err... sorry. To be honest I'm kind of used to reading long lines of code so I don't usually consider it a problem.
You obviously don't read these forums much. And just because you like it doesn't mean we do ;) Most of us professionals have stricter standards
The cases with the stings were an exception to that of course. Anyway, here's the code edited so you can read it more easily:

Code:

//The code below is what I think needs work. I don't think this much code should be // required for a single registry value What make you think that? A check and test for error does not seem to be "too much code," especially for the registry. * (BOOL *) &defaultVal = 0; CheckRegValue(hKey,"Test", REG_BINARY, (void *) &myappStatus->test, (void *) &defaultVal, sizeof(BOOL), sizeof(BOOL), notFoundIsError, &errorMB); if(myappStatus->test > 1 || myappStatus->test < 0) //Invalid value test { myappStatus->test = * (BOOL *) &defaultVal; ErrorMessage("Registry value \"Test\" was invalid. Default value used", NULL, NULL, MB_ICONEXCLAMATION, 1); } return; }

Ditto the points on long lines.
It's hard enough to spot bugs when you can see the whole code on screen.
Long lines just give you something else to worry about which really ought not to be a worry at all.

> "\0"
Is the same as "", only it takes 2 bytes to say it instead of 1.

And what's with all the casting. It's got that C++ feeling all over it.
> free((void *) mergedString[0]);
There is no way that cast is necessary in C

I'm not sure what this is:

Code:

BYTE defaultVal[MAX_PATH]; ... * (BOOL *) &defaultVal = 0;

If BOOL is an int, you're setting the first 4 values of the array to zero. Can you explain why?

Initializing the array can be done like this:

Code:

BYTE defaultVal[MAX_PATH] = {0};

Quote:

Originally posted by WaltP
What make you think that? A check and test for error does not seem to be
"too much code," especially for the registry.

Well my main concern is that the code get's really redundant when you have 20 or so reg values to read. I was hoping there would be a way to impelement al that code into a single function, but I couldn't think of one.

Quote:

Originally posted by Salem
Ditto the points on long lines.
It's hard enough to spot bugs when you can see the whole code on screen.
Long lines just give you something else to worry about which really ought not to be a worry at all.

> "\0"
Is the same as "", only it takes 2 bytes to say it instead of 1.

And what's with all the casting. It's got that C++ feeling all over it.
> free((void *) mergedString[0]);
There is no way that cast is necessary in C

Ah, good idea on the /0 thing. The type cast however, is neccesary since free() requires a void * type to be passed to it. If I give it anything else then I get an annoying compiler warning.

Quote:

Originally posted by Monster
I'm not sure what this is:

Code:

BYTE defaultVal[MAX_PATH]; ... * (BOOL *) &defaultVal = 0;

If BOOL is an int, you're setting the first 4 values of the array to zero. Can you explain why?

Initializing the array can be done like this:

Code:

BYTE defaultVal[MAX_PATH] = {0};

This example was just to illustate the standard form for setting a reg value I worked out. In this particular case I could have done it that way, however if the default value was anything else, which it very likely could be, then it wouldn't work. Anyway, for this particular piece of code I'm more worried about keeping it uniform than making it fast.

Rough suggestion:

Code:

#include <windows.h> #include "config.h" #include "debug.h" #include "global_definitions.h" #include "memory.h" #include "numio.h" #include "stringio.h" MYAPPSTATUS g_appStatus; // Functions prototypes ///////// void InitAppStatus(MYAPPSTATUS * myappStatus); LONG CheckRegValue(HKEY hKey, LPTSTR regValue, DWORD dataType, void * configPointer, DWORD defValueSize, DWORD maxValueSize, BOOL notFoundIsError, BOOL * errorMB); #define NOTE_INVALID_REGISTRY_VALUE(szValueName) \ ErrorMessage("Registry value \"" szValueName "\" was invalid. Default value used", NULL, NULL, MB_ICONEXCLAMATION, 1); // Function definitions /////// void InitMyappStatus(MYAPPSTATUS * myappStatus) { DWORD dwKeyDisposition; BOOL bNotFoundIsError; BOOL bErrorMB = 1; HKEY hKey; DWORD dwDefault; RegCreateKeyEx( HKEY_CURRENT_USER, myapp_DEFAULT_REGKEY, 0, TEXT(""), REG_OPTION_NON_VOLATILE, KEY_READ | KEY_WRITE, (LPSECURITY_ATTRIBUTES) NULL, &hKey, &dwKeyDisposition); bNotFoundIsError = (dwKeyDisposition == REG_OPENED_EXISTING_KEY); // a bool or dword value... dwDefault = MY_DEFAULT; CheckRegValue(hKey,"Test", REG_DWORD, &myappStatus->btest, &dwDefault, sizeof(BOOL), sizeof(BOOL), notFoundIsError, &errorMB); if(myappStatus->btest > 1 || myappStatus->btest < 0) { myappStatus->test = MY_DEFAULT; NOTE_INVALID_REGISTRY_VALUE("Test"); } // now a string... CheckRegValue(hKey, "StrTest", REG_SZ, myappStatus->szTest, "Default Value", sizeof(myappStatus->szTest), sizeof("Default Value"), notFoundIsError, &errorMB); if (invalidvalue) { lstrcpy(myappstatus->szTest, "Default Value"); NOTE_INVALID_REGISTRY_VALUE("StrTest"); } // etc... return; } LONG CheckRegValue(HKEY hKey, LPCTSTR szRegValue, DWORD dwDataType, LPVOID pvData, LPVOID pvDefault, DWORD cbMaxData, DWORD cbDefault, BOOL notFoundIsError, BOOL * errorMB) { LONG error; error = RegQueryValueEx(hKey, szRegValue, NULL, NULL, pvData, &cbMaxData) if (error != ERROR_SUCCESS) { if(error == ERROR_FILE_NOT_FOUND) { if(notFoundIsError) { *errorMB = (IDOK == ErrorPrintf("Failed to Read Registry Key", MB_ICONEXCLAMATION | MB_OKCANCEL, *errorMB, "Couldn't find registry value: %s. If you have not deleted any %s" "keys then you may have a corrupted registry. Press OK to continue or Cancel " "to continue and skip any subsequent registy errors.", regValue, VERSION) ); } else { ErrorPrintf("Registy value initialized.", MB_ICONINFORMATION, 0, "The following registry value was successfully initialized: %s", regValue); } } else { *errorMB = (IDOK == ErrorPrintf("Failed to Read Registry Key", MB_ICONEXCLAMATION | MB_OKCANCEL, *errorMB, "Failed to read registry value \"%s\". Error code: %d." "Press OK to continue or Cancel to continue and skip any subsequent registy errors.", regValue, error) ); } RegSetValueEx(hKey, regValue, 0, dataType, (CONST BYTE *) pvDefault, cbDefault); memcpy(pvData, pvDefault, min(cbData, cbDefault)); } return error; }

I think your error reporting is probably execessive in some places while missing in much more important places like creating the registry key in the first place. As a guide, here is the registry code for a recent project of mine:

Code:

/* ============================================== */ void Registry_LoadData(void) { TCHAR szRegKey[MAX_REG_NAME]; HKEY hKey; HRESULT hr; LONG lr; GetRegistryKeyName(szRegKey); /* Open the job specific registry key */ lr = RegOpenKeyEx(HKEY_CURRENT_USER, szRegKey, 0, KEY_QUERY_VALUE, &hKey); if (ERROR_SUCCESS != lr) { ReportError(MSG_ERR_OPEN_REG_KEY, lr, ET_WARNING2, __FILE__, __LINE__, szRegKey); return; } /* Get the last backup time, incremented id number and the number of files that were found during the last backup */ hr = RegGetBinary(hKey, TEXT("Last Backup Time"), &pbj->ftLastBackup, sizeof(pbj->ftLastBackup)); hr |= RegGetBinary(hKey, TEXT("Next Id Number"), &pbj->nBackupId, sizeof(pbj->nBackupId)); hr |= RegGetBinary(hKey, TEXT("LastFoundFiles"), &pbj->cLastFoundFiles, sizeof(pbj->cLastFoundFiles)); if (NOERROR != hr) ReportError(MSG_ERR_GET_REG_VALUE, hr, ET_WARNING1, __FILE__, __LINE__, szRegKey); if (0 == pbj->cLastFoundFiles) pbj->cLastFoundFiles = 100; /* Default estimate */ RegCloseKey(hKey); } /* ============================================== */ void Registry_SaveData(void) { TCHAR szRegKey[MAX_REG_NAME]; HKEY hKey; LONG lr; DWORD dwBackupId = pbj->nBackupId + 1; GetRegistryKeyName(szRegKey); lr = RegCreateKeyEx(HKEY_CURRENT_USER, szRegKey, 0, NULL, REG_OPTION_NON_VOLATILE, KEY_SET_VALUE, NULL, &hKey, NULL); if (ERROR_SUCCESS != lr) { ReportError(MSG_ERR_CREATE_REG_KEY, lr, ET_WARNING1, __FILE__, __LINE__, szRegKey); return; } lr = RegSetValueEx(hKey, TEXT("Last Backup Time"), 0, REG_BINARY, (LPBYTE) &pbj->ftSnapped, sizeof(pbj->ftSnapped)); lr |= RegSetValueEx(hKey, TEXT("Next Id Number"), 0, REG_DWORD, (LPBYTE) &dwBackupId, sizeof(dwBackupId)); lr |= RegSetValueEx(hKey, TEXT("LastFoundFiles"), 0, REG_DWORD, (LPBYTE) &pbj->cFoundFiles, sizeof(pbj->cFoundFiles)); if (ERROR_SUCCESS != lr) ReportError(MSG_ERR_SET_REG_VALUE, lr, ET_WARNING1, __FILE__, __LINE__, szRegKey); RegCloseKey(hKey); }

Note that the error messages are stored in a message file and are not polluting the code.

Quote:

Originally posted by Xzyx987X
This example was just to illustate the standard form for setting a reg value I worked out. In this particular case I could have done it that way, however if the default value was anything else, which it very likely could be, then it wouldn't work. Anyway, for this particular piece of code I'm more worried about keeping it uniform than making it fast.

This is not about making it fast, it's about making your program solid. You only initialise the first 4 bytes in the array. This can be very dangerous!

Quote:

The type cast however, is neccesary since free() requires a void * type to be passed to it.

free() requires a pointer to be passed, it can be of any type. A void pointer is a pointer that points to a location without knowing what type of data is contained there.

The correct method would be:

Code:

free(mergedString[0]);

If your compiler is complaining about that then you might wish to look for a new compiler.

The only reason the compiler is complaining is because they're using a C++ compiler to compile C code.

Well I think it's C code, there's so much windows'y stuff in there it's hard to tell.
VC++ defaulting to a file type of .cpp doesn't help matters either.

Like I said time to look for a better compiler

/ducks

Quote:

Originally posted by Monster
This is not about making it fast, it's about making your program solid. You only initialise the first 4 bytes in the array. This can be very dangerous!

Not really. The only time it would be a problem is if I forgot to set the default value, or used the wrong size for the type of value, in which case I'd have a bug whether it was initialized or not anyway.

Quote:

Originally posted by Salem
The only reason the compiler is complaining is because they're using a C++ compiler to compile C code.

Well I think it's C code, there's so much windows'y stuff in there it's hard to tell.
VC++ defaulting to a file type of .cpp doesn't help matters either.

Yes, i am using VC++, yes I did make it a .cpp file, and yes I did intend to write my program in pure C. Maybe that's not such a good idea, but I really didn't want to have to give up function overloading.