legacy code and exception handling

Question

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

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;
}

Answer 1

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 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<char>. In such a case, to get the pointer out of it, use &buffer[0] when you pass it to the IniPersistIf methods.

Answer 2

All those functions: 'GetFullInifileName', 'Read' and 'Write' should provide no-throw guarantee. otherwise it will lead to memory leak and even results to undefined behavoir if exception is thrown from the destructor. Right now these functions refferring to Windows API C functions like GetPrivateProfileString, WritePrivateProfileString, etc. and in C there is no such exception idiom. To some extent the 'IniValue' class is 'safe', but do not provide any error handling mechanism; typically C can manage such situation with last error number returned ('errno.h'). IniValue class designer made a 'Write' function call in destructor! There is no way to return information to the user of IniValue class about success or failure of 'write' operation (remmember, do not throw exception from destructor.) By my opinion IniValue class is dumb, brittle and still in production as legacy code. For how long?