HiveBrain v1.2.0
Get Started
← Back to all entries
debugcppMinor

legacy code and exception handling

Submitted by: @import:stackexchange-codereview··
0
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

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 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.