Parsing .obj 3D meshes

I have a legacy code parsing .obj 3D mesh files. However, it seems to me a bit ugly:

```
char Prefix[7];

float X, Y, Z;
int A1, A2, A3, B1, B2, B3, C1, C2, C3, D1, D2, D3;

while ( !IStream->Eof() )
{
Luint64 Pos = IStream->GetFilePos();
LString Line = IStream->ReadLine();

int NumRead = sscanf( Line.c_str(), "%6s %f %f %f", Prefix, &X, &Y, &Z );

if ( NumRead FileSystem->FindFile( LString(FileName), IStream->GetVirtualFileName() );
LoadOBJMaterials( MatName, File );
}
else if ( strcmp( Prefix, "usemtl" ) == 0 )
{
if ( Mesh->FFaces.size() )
{
// start a new mesh
IStream->Seek( Pos );
return true;
}
char MtlName[33];
sscanf( Line.c_str(), "%6s %32s", Prefix, MtlName );
Mesh->FUseMtl = LString( MtlName );
}
else switch ( Prefix[0] )
{
case '#':
continue;
case 'v':
{
switch ( Prefix[1] )
{
case 0:
if ( NumRead != 4 ) continue;
File->EmitVertex( LVector3( X, Z, Y ) );
break;
case 't':
if ( NumRead EmitTexCoord( LVector2( X, 1.0f-Y ) );
break;
case 'n':
if ( NumRead != 4 ) continue;
File->EmitNormal( LVector3( X, Z, Y ) );
break;
}
}
break;
case 'f':
{
if ( sscanf( Line.c_str(), "%2s %d/%d %d/%d %d/%d %d/%d", Prefix, &A1, &A2, &B1, &B2, &C1, &C2, &D1, &D2 ) == 9 )
{
A3 = B3 = C3 = 0;
sOBJFace Face;
Face.Vt1 = A2;
Face.Vt2 = B2;
Face.Vt3 = D2;
Face.V1 = A1;
Face.V2 = B1;
Face.V3 = D1;
Mesh->EmitFace( Face );

Face.Vt1 = B2;
Face.Vt2 = C2;
Face.Vt3 = D2;

Despite the fact that the code is incomplete, it is still reviewable
as a standalone function. And I agree with you, it is quite ugly.

Poor variable naming:

These variables:

float X, Y, Z;
int A1, A2, A3, B1, B2, B3, C1, C2, C3, D1, D2, D3;

are being used and abused through the function. They are being used
to hold values for different things at different points. This
reuse attempt is probably the reason for the undescriptive names.

Variables should be declared in their point of use, with the intent
of minimising scope. And of course, you need to give them more
meaningful names than the letters of the alphabet.

sscanf():

I don't fully condemn the use of sscanf() in this case, since
I think it is a decent string parsing function, and quite readable too.

To replace it, you would probably have to switch to std::istringstream.
That would involve a lot of change and possibly not much gain readability-wise.

But one possible problem with sscanf() in your code is the size of the
Prefix array. 7 is a very small buffer size (remember that the last index must
be the '\0', so you only really have 6 chars to use). Living this
close to the edge is silly. Just make the buffer a bit larger.

As a consequence of using a char[], you are forced to use strcmp(),
which might not be very clear to someone without a C background.
One way to improve readability here would be to copy Prefix to
an std::string after sscanf() returns and then replace the
strcmps with the == string operator.

if ( prefix == "mtllib" )

Excessive large function / code block:

The code block you've presented here is too large. And it might not
even be the whole contents of the given function. It needs some
refactoring. You should split it into a few helper methods. Conceptually, it should look something like this:

while ( !IStream->Eof() )
{
    ...

    if ( prefix == "mtllib" )
    {
        ParseMTLLIB(...);    
    }
    else if ( prefix == "usemtl" )
    {
        ParseUSEMTL(...);
    }
    else 
    {
        switch ( prefix[0] )
        {
        case '#': // skip comment
            continue;
        case 'v':
            ParseVertex(...);
            break;
        case 'f': 
            ParseFace(...);
            break;
        default:
            break;
        }
    }
}

Missing a default in the switch:

The switch() statement was missing the default case. You should always
add one, even if it is a fall-through.

A word on naming convention:

One last suggestion regarding naming. It is unusual to name variables
and type instances in C++ using a first letter in upper case, LikeThis.

A more usual naming for variables is camelCase, with the first in
lower-case. CamelCase names are preferred for use with type names,
such as is the case in your LString type, for example.

Another thing, prefixing a struct or class name, such as with sOBJFace
is frowned upon by some. Personally, I don't see the need for that s prefix, as
OBJFace would be very descriptive and clear.

StackExchange Code Review Q#64146, answer score: 2