Error handling for xml parsing

Question

I'm using tinyxml to parse xml files, and I've found that error handling here lends itself to arrow code. Our error handling is simply reporting a message to a file.

Here is an example:

  const TiXmlElement *objectType = dataRoot->FirstChildElement( "game_object" );
  if ( objectType ) {
    do {
      const char *path = objectType->Attribute( "path" );
      if ( path ) {
        const TiXmlElement *instance = objectType->FirstChildElement( "instance" ); 
        if ( instance ) {
          do {
            int x, y = 0; 
            instance->QueryIntAttribute( "x", &x );
            instance->QueryIntAttribute( "y", &y );
            if ( x >= 0 && y >= 0 ) {
              AddGameObject( new GameObject( path, x, y ));
            } else {
              LogErr( "Tile location negative for GameObject in state file." );
              return false;
            }
          } while ( instance = instance->NextSiblingElement( "instance" ));
        } else {
          LogErr( "No instances specified for GameObject in state file." );
          return false;
        }
      } else {
        LogErr( "No path specified for GameObject in state file." );
        return false;
      }
    } while ( objectType = objectType->NextSiblingElement( "game_object" ));
  } else {
    LogErr( "No game_object specified in <game_objects>. Thus, not necessary." );
    return false;
  }
  return true;

I'm not huffing and puffing over it, but if anyone can think of a cleaner way to accomplish this it would be appreciated.

P.S. Exceptions not an option.

Edit:

Would something like this be preferable?

if ( !path ) {
  // Handle error, return false
}
// Continue

This eliminates the arrow code, but the arrow code kind of puts all of the error logging on one place.

Answer 1

You could create a macro for that, which encapsulates the if (!var) { .. return false; } and error reporting.

However, I do not see how this can be improved all that much; its just the way it is. C'est la vie. C'est le code...

Answer 2

Using return values as error codes just leads to such code, it can't be improved much. A slightly cleaner way would use goto to group all error handling into a single block and to decrease the nesting of blocks.

This does however not solve the actual problem, which is using return values as error codes. In C, there is no alternative, but in C++ exceptions are available and should be used. If they are not an option, you're are stuck with what you have.

Answer 3

I'm not huffing and puffing over it, but if anyone can think of a cleaner way to accomplish this it would be appreciated.

I have replaced the nested ifs with return statements on error (this makes the code "flow down" instead of going "arrow shaped". I have also replaced your do loopps with for loops (so I could understand it better).

Is this what you wanted?

const TiXmlElement *objectType = dataRoot->FirstChildElement( "game_object" );
if ( !objectType ) {
    LogErr( "No game_object specified in <game_objects>. Thus, not necessary." );
    return false;
}

for(; objectType != 0; objectType = objectType->NextSiblingElement( "game_object" )) {
    const char *path = objectType->Attribute( "path" );
    if ( !path ) {
        LogErr( "No path specified for GameObject in state file." );
        return false;
    }

    const TiXmlElement *instance = objectType->FirstChildElement( "instance" ); 
    if ( !instance ) {
        LogErr( "No instances specified for GameObject in state file." );
        return false;
    }

    for(; instance != 0; instance = instance->NextSiblingElement( "instance" )) {
        int x, y = 0; 
        instance->QueryIntAttribute( "x", &x );
        instance->QueryIntAttribute( "y", &y );
        if ( x >= 0 && y >= 0 ) {
            AddGameObject( new GameObject( path, x, y ));
        } else {
            LogErr( "Tile location negative for GameObject in state file." );
            return false;
        }
    }
}
return true;