Recommended dealing with while loop errors

Question

In a situation where a text file is read line by line through a while loop, what is the recommended method for dealing with errors?

(note: fggets from here is used)

bool SomeClass::ReadFile(int ignorelines) {
    FILE *file = fopen(m_filepath.c_str(), "r");
    if (file!=NULL) {
        char *buffer;                           //line reading buffer
        while (fggets(&buffer,file)==0) {       //read line (fggets returns non-zero at provided terminator or \eof)
            if (ignorelines>0) {
                ignorelines--;                  //count the lines ignored
                free(buffer);
                continue;
            }
            line.assign(buffer);
            if (!PerformLineCheck1(buffer)) {   
                m_error+="The line is not allowed.\n";
                free(buffer);
                fclose(file);
                return false;
            }
            if (!PerformLineModification1(buffer)) {
                m_error+="The line could not be modified properly.\n";
                free(buffer);
                fclose(file);
                return false;
            }
            if (!PerformLineExtraction(buffer)) {   
                m_error+="The variables could not be extracted.\n";
                free(buffer);
                fclose(file);
                return false;
            }
            //extracted all, now continue with next line
            free(buffer);                       //clear up the line
        }
        fclose(file);
        return true;
    }
    m_error+="There was an error opening the data file.\n";
    return false;
}

There are some options I could think of:

• use break; to get out of the while loop: but then I would need to add free(buffer); which can cause freeing memory twice.
• use try and catch, however this deals with all other exceptions in those functions as well and suggested was to use assert (as suggested here). However, assert will assume code errors and break operation, while for this situation program execution can continue, the function simply needs to return false.

What is the suggestion for something like the loop above?

Answer 1

First, try and catch is not an entirely bad idea. You can test for specific errors and rethrow the rest, or catch every possible error in one dragnet, print an error message, and close up. It entirely depends on the type of data you're reading, what errors you will reasonably expect to see, and what level of gracious error recovery you need to do.

As for using break, there is a way to do this without doing a double free, however, it might be unpopular with some people.

Essentially, it looks something like this:

while (someCondition) {
   // Do things
   error = 1; // Error encountered
   if (error)
       goto cleanUp;
   // Do some more things
}
// Do some other things that you need to do outside the loop here
:cleanUp
free(buffer);
return error;

It operates similar to having a custom onError event handler in some languages.

Answer 2

Step one: Deal with errors early, so move the "if we couldn't open the file" up a bit.

bool SomeClass::ReadFile(int ignorelines) {
    FILE *file = fopen(m_filepath.c_str(), "r");
    if (file==NULL) {
      m_error+="There was an error opening the data file.\n";
      return false;
    }

Then the rest of the code use a res variable to track whether the whole operation succeeded or not, and a single if at the end of the sequence to determine if we continue the loop or not (or you could do while (res && fggets( ... )) if you prefer that).

    char *buffer;                           //line reading buffer
    bool result = true;
    while (fggets(&buffer,file)==0) {       //read line (fggets returns non-zero at provided terminator or \eof)
        if (ignorelines>0) {
            ignorelines--;                  //count the lines ignored
        }
        else
        {
            line.assign(buffer);
            if (!PerformLineCheck1(buffer)) {   
                m_error+="The line is not allowed.\n";
               res = false;
            }
            else if (!PerformLineModification1(buffer)) {
                m_error+="The line could not be modified properly.\n";
                res = false;
            } 
            else if (!PerformLineExtraction(buffer)) {   
                m_error+="The variables could not be extracted.\n";
                res = false;
            }
        }
        //extracted all, now continue with next line
        free(buffer);                       //clear up the line
        if (!res) break; 
    }
    fclose(file);
    return res;
}

That reduces the duplication in the code to free and close.

(Edit: Not being a fan of continue, I changed it to else after the first if, and removed that free(buffer) call too)

Answer 3

1. You can use the try block with just one line, so other functions would not cause any problem, example is shown below.
2. If you are unsure if a certain buffer is cleared, use the xxx==nullptr to check so it will not be deleted twice.

For point no. 1, If you want to catch the exception while using fgets(), then do this:

int result = 0;
while (true) {
    char* buffer = new char[100]; //YOU MUST allocate space for the buffer, 
                                  //cannot use it directly
    try {
        result = fggets(&buffer,file);
    }
    catch () {
        //do whatever you need to catch, break if necessary
        //handling of memory is shown below
    }
    if (0 == result) {
        if (nullptr != buffer) { // make sure when you execute delete[] or free, the pointer is valid
            delete[] buffer;//or free
        }
        break;
    }
    //do your other things here
}