1
\$\begingroup\$

I'm hoping for a little bit of a code review to ensure I am doing UTF-8 right. I am developing a "csv_fixer" that allows a user to select the encoding of the file so we can correctly handle encoding errors. Also, there is a choice to skip this, in which case I am just removing the fields that have encoding errors.

So what I need is a bit of a review just to be absolutely sure that I am doing this right. I know encoding is almost an impossible problem but hopefully I can be told otherwise.

My logic is as follows. At the beginning, I read the source CSV file counting the number of rows with invalid UTF8 characters using my bool isUTF8() shown below. I collect either 200 rows with encoding errors, or all encoding errors of the file. Whichever comes first. These rows are then shown to the user, and they are given a screen that allows them to pick the encoding of the file, showing the data as the choose. Once this is done, the file is read from the beginning as the selected encoding. Any encoding errors that were encountered as the selected encoding are written to an encoding errors file. At the end of parsing, the user can choose the encoding of those items until all items are written to the output file.

Now this implementation seems to correctly ensure only valid UTF8 is written to the file, but it seems that a majority of the data with encoding errors just gets replaced by a replacement character that is valid UTF8 defeating the purpose. And if I just skip this step and choose to remove all fields with encoding errors, it seems to work perfect, except for Windows in which case badly encoded characters are being replaced with '?'.

Anyways, here is the implementation of my string encoder. This function gets the std::string row of the .csv file.

inline QString encode_string( std::string str, std::string encoding )
{
    QByteArray encoded;

    if ( encoding == UTF8 ) {
        encoded = QString::fromStdString( str ).toUtf8();
    }  else if ( encoding == ISO88591 ) {
        QTextCodec *codec = QTextCodec::codecForName( ISO88591 );
        QByteArray enc( str.c_str(), str.length() );
        return QString( codec->toUnicode( enc ) );
    } else if ( encoding == ISO88592 ) {
        QTextCodec *codec = QTextCodec::codecForName( ISO88592 );
        QByteArray enc( str.c_str(), str.length() );
        return QString( codec->toUnicode( enc ) );
    } else if ( encoding == WINDOWS1251 ) {
        QTextCodec *codec = QTextCodec::codecForName( WINDOWS1251 );
        QByteArray enc( str.c_str(), str.length() );
        return QString( codec->toUnicode( enc ) );
    } else if ( encoding == WINDOWS1252 ) {
        QTextCodec *codec = QTextCodec::codecForName( WINDOWS1252 );
        QByteArray enc( str.c_str(), str.length() );
        return QString( codec->toUnicode( enc ) );
    } else if ( encoding == SHIFTJIS ) {
        QTextCodec *codec = QTextCodec::codecForName( SHIFTJIS );
        QByteArray enc( str.c_str(), str.length() );
        return QString( codec->toUnicode( enc ) );
    } else if ( encoding == EUCKR ) {
        QTextCodec *codec = QTextCodec::codecForName( EUCKR );
        QByteArray enc( str.c_str(), str.length() );
        return QString( codec->toUnicode( enc ) );
    } else if ( encoding == EUCJP ) {
        QTextCodec *codec = QTextCodec::codecForName( EUCJP );
        QByteArray enc( str.c_str(), str.length() );
        return QString( codec->toUnicode( enc ) );
    } else {
        qDebug() << Q_FUNC_INFO  << "Hit bad encoding case.";
        return QString( encoded );
    }
}

Here is the implementation of my valid utf8 checker

 bool Parser::isUTF8( std::string string )
 {
    QString utf8str = QString::fromUtf8( string.c_str() );

    for ( int i = 0; i < utf8str.length(); i++ ) {
        if ( utf8str.at( i ) == -3 ) {
            return false;
        }

       return true;
 }

And here is my call point:

    //Write utf8 version of the string
    QString encoded = util::encode_string( joined, this->encoding );

    //If the encoded string has UTF8 errors, write it to the encode error file
    if ( !isUTF8( encoded.toStdString() ) ) {
        QFile encode( this->encodeErrFileName );
        encode.open( QIODevice::ReadWrite | QIODevice::Append );
        QTextStream encodeOut( &encode );
        encodeOut << encoded << "\r\n";
        encode.close();
    } else {
        emit cleanRow();
        tmpFileWriter << encoded << "\r\n";
    }

    output.close();

My function calls go like this:

Read file as standard (C++ ifstream, no encoding done) -> encode the string (using util::encode_string) -> check if encoded string is valid UTF8 (using bool isUTF8(str)) -> if true, write to output file, if false write to encoding errors file.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Not sure if you have noticed, but in encode_string() the first if path will not return a value, leaving you with undefined behavior.

The if-then-else chain in encode_string() is also unnecessary, since the value you compare is the same that is passed to QTextCodec::codecForName(). The function could be as simple as this:

inline QString encode_string( const std::string & str, const std::string & encoding )
{
    if ( encoding == UTF8 ) 
    {
        return QString( QString::fromStdString( str ).toUtf8() );
    }  

    QTextCodec *codec = QTextCodec::codecForName( encoding );
    if (!coded)
    {
        qDebug() << Q_FUNC_INFO  << "Hit bad encoding case.";
        return QString( QString::fromStdString( str ).toUtf8() );
    }

    QByteArray enc( str.c_str(), str.length() );
    return QString( codec->toUnicode( enc ) );
}

Since the UTF8 case was missing the return, I've guessed what you wanted to return there. I would also take the parameters by const reference, to avoid unnecessary copies.

Also, should the QTextCodec pointer returned by QTextCodec::codecForName() be freed after use? I dunno, check the documentation. That might be leaking memory.

\$\endgroup\$
3
  • \$\begingroup\$ I did actually notice that after the fact and changed it. But I will definitely be putting this in my code base. Much cleaner! Thanks for taking a look. \$\endgroup\$
    – k_dog345
    Apr 3, 2015 at 13:50
  • \$\begingroup\$ One question though, why are you having to inner calls to QString in the return statements? return QString( QString::fromStdString( str ).toUtf8() ); \$\endgroup\$
    – k_dog345
    Apr 3, 2015 at 13:52
  • \$\begingroup\$ Never mind, I know. It's because QString::toUtf8() returns a QByteArray. Sorry should have read the docs before asking :P \$\endgroup\$
    – k_dog345
    Apr 3, 2015 at 13:54

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.