3
\$\begingroup\$

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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ If it is not broken then don't try and fix it. But feel free to add unit tests. \$\endgroup\$ – Martin York Mar 8 '13 at 15:05
  • \$\begingroup\$ constructor does not initialize defaultval, but the destructor frees it. \$\endgroup\$ – fork0 Mar 8 '13 at 16:16
  • \$\begingroup\$ If the code in IniValue::TheGetProfileString() between new and delete can throw an exception - you have a memleak (defstr). \$\endgroup\$ – fork0 Mar 8 '13 at 16:17
  • \$\begingroup\$ @fork thanks for correction. 'defaultval' was accidental copy-paste mistake, I've just removed it from the code. \$\endgroup\$ – sigidagi Mar 9 '13 at 7:05
  • \$\begingroup\$ @Loki, sure, I'll have your comment in mind. My intension is to understand what C++ software engineers will do differently, than they did 20 years ego. \$\endgroup\$ – sigidagi Mar 9 '13 at 7:19
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Can those Windows functions: WritePrivateProfileString, GetPrivateProfileString, CreateDirectory, etc. throw an exception? \$\endgroup\$ – sigidagi Mar 8 '13 at 12:13
  • \$\begingroup\$ If they are C functions they cannot throw as there are no exceptions in C. \$\endgroup\$ – CashCow Dec 17 '13 at 15:24
0
\$\begingroup\$

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?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.