debugcppMinor
legacy code and exception handling
Viewed 0 times
handlingexceptionandcodelegacy
Problem
After some 'digging' into the dark corners of legacy code I've found class, which handles INI files. It does reading and writing to the file, but I haven't found any exception handling logic. What kind of potential problems, if any, can you detect and what is your suggestion/advise for improving the code, based on these three C++ code snippets:
Constructor
Some function, which reads values from the file:
Destructor
Constructor
IniValue::IniValue(const char* FileName, const char* Section, const char* Entry, int sz, PersistMediumEnum medium_, int iniprofileId_)
:filename(NULL)
,section(new char[strlen(Section)+1])
,entry(new char[strlen(Entry)+1])
,buffer(new char[sz])
,bufsiz(sz)
{
if (FileName == NULL) {
FileName = "somefile.ini";
}
// 'GetFullInifileName' calls windows functions 'GetEnvironmentVariable', 'CreateDirectory', 'CopyFile', 'DeleteFile'
std::string fullname = GetFullInifileName(FileName);
filename = new char[fullname.length()+1]; strcpy(filename, fullname.c_str());
strcpy(section, Section);
strcpy(entry, Entry);
buffer[0] = 0;
}Some function, which reads values from the file:
void IniValue::TheGetProfileString() {
char* defstr = new char[bufsiz];
strncpy(defstr, buffer, bufsiz);
IniPersistIF* inf = IniPersistIF::get(medium);
//calls windows 'GetPrivateProfileString'
inf->Read(section, entry, defstr, buffer, bufsiz, filename, iniprofileId);
delete [] defstr;
}Destructor
IniValue::~IniValue() {
IniValue* thys = this;
assert(section[0] != '\0');
assert(filename[0] != '\0');
IniPersistIF* inf = IniPersistIF::get(medium);
assert(inf != NULL);
assert(section[0] != '\0');
assert(filename[0] != '\0');
// Calls Windows 'WritePrivateProfileString' function
inf->Write(section, entry, buffer, filename, iniprofileId, defaultval, bModified);
delete [] buffer;
delete [] entry;
delete [] section;
delete [] filename;
}Solution
This was posted just a short while ago on StackOverflow.com
Firstly, if this is legacy code that has existed in production for a long time, then even if you can make it semantically better, you have to really weigh up the gains against the losses. Changing code is a cost, can introduce bugs even when the new code appears to be a lot better, and has to be justified. That the existing code uses a lot of arrays does not mean it has any bugs, nor would the "improvements" give you better performance in any way.
However, on the basis of giving this code a review I will do so.
Basically, start off by getting rid of all the arrays and calls to new[] and delete[] and use std::string (which is used there in one place so it is obviously known to the programmer).
Ensure your IniPersistInf class is const-correct so takes if it takes a string that it only reads, it uses
Firstly, if this is legacy code that has existed in production for a long time, then even if you can make it semantically better, you have to really weigh up the gains against the losses. Changing code is a cost, can introduce bugs even when the new code appears to be a lot better, and has to be justified. That the existing code uses a lot of arrays does not mean it has any bugs, nor would the "improvements" give you better performance in any way.
However, on the basis of giving this code a review I will do so.
Basically, start off by getting rid of all the arrays and calls to new[] and delete[] and use std::string (which is used there in one place so it is obviously known to the programmer).
Ensure your IniPersistInf class is const-correct so takes if it takes a string that it only reads, it uses
const char or const std::string & rather than char .buffer in IniValue appears to be for writing into, so for this one you might use std::vector. In such a case, to get the pointer out of it, use &buffer[0] when you pass it to the IniPersistIf methods.Context
StackExchange Code Review Q#23615, answer score: 2
Revisions (0)
No revisions yet.