5
\$\begingroup\$

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 < 1 ) continue;

    if ( strcmp( Prefix, "mtllib" ) == 0 )
    {
        char FileName[33];
        sscanf( Line.c_str(), "%6s %32s", Prefix, FileName );
        LString MatName = Env->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 < 3 ) continue;
                File->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;
                Face.V1 = B1;
                Face.V2 = C1;
                Face.V3 = D1;
                Mesh->EmitFace( Face );
            }
            else if ( sscanf( Line.c_str(), "%2s %d/%d/%d %d/%d/%d %d/%d/%d %d/%d/%d", Prefix, &A1, &A2, &A3, &B1, &B2, &B3, &C1, &C2, &C3, &D1, &D2, &D3 ) == 13 )
            {
                sOBJFace Face;
                Face.Vt1 = A2;
                Face.Vt2 = B2;
                Face.Vt3 = D2;
                Face.Vn1 = A3;
                Face.Vn2 = B3;
                Face.Vn3 = D3;
                Face.V1 = A1;
                Face.V2 = B1;
                Face.V3 = D1;
                Mesh->EmitFace( Face );

                Face.Vt1 = B2;
                Face.Vt2 = C2;
                Face.Vt3 = D2;
                Face.Vn1 = B3;
                Face.Vn2 = C3;
                Face.Vn3 = D3;
                Face.V1 = B1;
                Face.V2 = C1;
                Face.V3 = D1;
                Mesh->EmitFace( Face );
            }
            else if ( sscanf( Line.c_str(), "%2s %d/%d/%d %d/%d/%d %d/%d/%d", Prefix, &A1, &A2, &A3, &B1, &B2, &B3, &C1, &C2, &C3 ) == 10 )
            {
                sOBJFace Face;
                Face.Vt1 = A2;
                Face.Vt2 = B2;
                Face.Vt3 = C2;
                Face.Vn1 = A3;
                Face.Vn2 = B3;
                Face.Vn3 = C3;
                Face.V1 = A1;
                Face.V2 = B1;
                Face.V3 = C1;
                Mesh->EmitFace( Face );
            }
            else if ( sscanf( Line.c_str(), "%2s %d//%d %d//%d %d//%d", Prefix, &A1, &A3, &B1, &B3, &C1, &C3 ) == 7 )
            {
                A2 = B2 = C2 = 0;
                sOBJFace Face;
                Face.V1 = A1;
                Face.V2 = B1;
                Face.V3 = C1;
                Face.Vn1 = A3;
                Face.Vn2 = B3;
                Face.Vn3 = C3;
                Mesh->EmitFace( Face );
            }
            else if ( sscanf( Line.c_str(), "%2s %d/%d %d/%d %d/%d", Prefix, &A1, &A2, &B1, &B2, &C1, &C2 ) == 7 )
            {
                A3 = B3 = C3 = 0;
                sOBJFace Face;
                Face.Vt1 = A2;
                Face.Vt2 = B2;
                Face.Vt3 = C2;
                Face.V1 = A1;
                Face.V2 = B1;
                Face.V3 = C1;
                Mesh->EmitFace( Face );
            }

        }
        break;
    }
}

return false;

Is there a way to make this C++ code faster and/or cleaner?

\$\endgroup\$
3
  • 1
    \$\begingroup\$ This isn't even the whole function. So as long as you don't show a more complete version of your code: You can save yourself the A1-D3 variables and read directly into sOBJFace variables. \$\endgroup\$
    – user45891
    Sep 29, 2014 at 14:55
  • \$\begingroup\$ @user45891: reading directly into sOBJFace variables looks like a good idea! \$\endgroup\$
    – Sergey K.
    Sep 29, 2014 at 15:43
  • 1
    \$\begingroup\$ I have no idea what a 3D mesh file is. So tat make sit hard to comment. Also there is a whole bunch of type information missing (What is the type of IStream?). I also see a lot of sscanf() in there that make in unsafe and there is a set strcmp() which would also make me nervous. So this is not really C++ code. Looks more like C with a few classes thrown in. \$\endgroup\$ Sep 29, 2014 at 17:55

1 Answer 1

2
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks! The reason for char[] instead of std::string is dynamic allocation. Per every line of the input file. There could be ~1M lines. \$\endgroup\$
    – Sergey K.
    Sep 30, 2014 at 16:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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