Common Programming Mistakes

Common Programming Mistakes


Zack Rusin zack@kde.org
Till Adam adam@kde.org
Richard Moore rich@kde.org
editorial afnindar@kde.org

Intro
This document aims to combine the experience of many of the top KDE developers about Qt and KDE frameworks dos and dont's. The way they were usually passed on to the next generation was by letting the youngsters make the mistakes and then yell at them in public. After reading this document you should be a lot less fearful of having your code reviewed by one of the more grumpy KDE developers [ ] We will go over things, which are not necessarily "bugs" but which make the code, either slower or less readable.

General C++
In this section we'll guide through some of the more dusty corners of C++ which either tend to be misused or which people simply get wrong.
Anonymous namespaces vs statics
If you have a method in a class that does not access any members and therefor does not need an object to operate, make it static. If additionally it is a private helper function that is not needed outside of the file, make it a file-static function. That hides the symbol completely.

Symbols defined in a C++ anonymous namespace do not have internal linkage. Anonymous namespaces only give an unique name for that translation unit and that is it; they don't change the linkage of the symbol at all. Linkage isn't changed on those because the second phase of two-phase name lookup ignores functions with internal linkages. Also, entities with internal linkage cannot be used as template arguments.

So for now instead of using anonymous namespaces use static if you don't want a symbol to be exported.
NULL pointer issues
First and foremost: it's fine to delete a null pointer. So the constructs like:

if (ptr)
delete ptr;

are simply redundant.
Second of all you'll see null pointers marked with three types: 0, 0L and NULL. The argument against using NULL was that while C defines it as a 0 void pointer, C++ defines it to not be a 0 void pointer. All conforming C++ implementations will define NULL correctly so it's really not a problem. The argument for 0L was that it was handled correctly in variable argument functions, while 0 wasn't. Nowadays that's also an artifact.
It's more a question of getting used to something. As far as the code in CVS goes you'll see 0 used more commonly than NULL.
When you delete a pointer, make sure you also set it to 0. So the idiom is:

delete ptr;
ptr = 0;

Member variables
You'll encounter four major styles of marking member variables in KDE:
m_variable - lowercase m, underscore and the name of the variable starting with a lowercase letter
mVariable - lowercase m and the name of variable starting with a uppercase letter
variable_ - variable name starting with a lowercase letter and then an underscore
_variable - underscore and the name of variable starting with a lowercase letter. It's being often frowned upon.
As it often happens there's no one correct way of doing it, so remember to always follow the syntax used by the application/library to which you are committing.
Static variables
Try to limit the number of static variables used in your code, especially when committing to a library. Construction and initialization of large number of static variables really hurts the startup times.
Forward Declarations
You will reduce compile times by forward declaring classes when possible instead of including their respective headers. For example:

#include //bad
#include //bad
#include //bad
class SomeInterface
{
public:
virtual void widgetAction( QWidget *widget ) =0;
virtual void stringAction( const QString& str ) =0;
virtual void stringListAction( const QStringList& strList ) =0;
};

The above should look as follows:

class QWidget; //good
class QStringList; //good
class QString; //good
class SomeInterface
{
//as above
};

Iterators
There's few issues at hand here, so in no particular order:
Cache the return of the end() method call before doing iteration over large containers. For example:

QValueList container;
//code which inserts a large number of elements to the container
QValueListConstIterator end( container.end() );
for( QValueListConstIterator itr( container.begin() ); itr != end; ++itr ) {
}

This avoids the unnecessary creation of the temporary end() return object on each loop iteration, largely speeding it up.
Prefer to use const_iterators over normal iterators when possible. Containers, which are being implicitly shared often detach when a call to a non-const begin() or end() methods is made (QValueList is an example of such a container). When using a const_iterator also watch out that you're really calling the const version of begin() and end(). Unless your container is actually const itself this probably won't be the case, possibly causing an unnecessary detach of your container. So basically whenever you use const_iterator initialize them using constBegin()/constEnd() instead of begin()/end(), to be on the safe side.
Prefer to use pre-increment over post-increment operators on iterators. You'll avoid creating an unnecessary temporary object.
Data Structures
In this section we'll go over some our most common pet-peeves which affect data structures very commonly seen in Qt/KDE applications.
QPtrList
QPtrList, as the name describes, is a list of pointers. QPtrList's are not implicitly shared. That means that every time you have constructs like :

class SomeClass
{
public:
QPtrList returnList() const;
private:
QPtrList m_list;
};

each invocation of the returnList method will copy the full QPtrList by iterating over all of its elements and inserting them in the newly created QPtrList.
So we recommend to use QValueList instead of QPtrList. You'll loose the ability to auto delete the pointers on destruction of the list, but it's trivial to add code which does that in the destructor of your class.
QCString
One of the most common mistakes is using QCString::length() method in a loop. QCString::length() calls strlen on each invocation, so calling it in a loop, as in

QCString someCString = ...;
for( int i = 0; i < someCString.length(); ++i ) {
//Do something
}

Will result in exactly strlen(someCString)+1 calls of strlen(someCString).
If you have to use QCString, remember to cache the result of QCString::length() method if you're planning to use it in a loop.
QByteArray
Often in KDE code developers want to transform the QByteArray to a QCString. The code which is often tried looks as follows:

void someFunc( const QByteArray& data )
{
QCString str( data ); //this is wrong!
}

The trick here is that QByteArray don't have to be null terminated. This introduces a subtle problem of trying to create a non-null terminated string. The correct code for constructing QCString's from QByteArray's is: QCString str( data, data.length() + 1 );
QString
It is common to want to test to see if a QString is null. It is faster to do this using the isNull() method than by comparing to the static QString::null value.

if ( mystring.isNull() ) { // YES!
}
if ( mystring == QString:: null ) { // NO!
}

Another twist to this comes when you want to test for the empty string rather that a null string - again Qt provides a method for the job. In many cases this can be preferred to testing for null:

if ( mystring.isEmpty() ) { // YES!
}
if ( mystring == "" ) { // NO!
}

QString
If you're reading in a file it is faster to convert it from the local encoding to unicode (QString) in one go rather than line by line. This means that methods like QIODevice::readAll() are often a good solution, followed by a single QString instantiation.

For larger files consider reading a block of lines then performing the conversion, that way you get the opportunity to update your GUI. This can be accomplished either by using qApp->processEvents() or by reentering the event loop normally and using a timer to read in the blocks in the background.
QString
Pass QStrings as const QString&. Even though QString is implicitly shared it is still more efficient (and safer) to pass const references as opposed to objects by value. So the canonical signature of a method taking QString arguments would be:

void myMethod( const QString & foo, const QString & bar );

Actually, pretty much everything should be passed by const reference if possible.
QString
While QString is the tool of choice for many string handling situations there is one where it is particularly inefficient. If you are pushing about and working on data in QCStrings or QByteArrays take care not to pass it through methods which take QString parameters and then make QCStrings or QByteArrays from them again. For example:


QCString myData;
QString myNewData = mangleData( myData );

QString mangleData( const QString data )
{
QCString str = data.latin1();
// mangle
return QString(str);
}

The expensive thing happening here is the conversion to QString which does conversion to unicode internally. That is unnecessary as the first thing the method does is convert back to latin1(). So if you are sure that the unicode conversion is not needed, try to avoid inadvertedly using QString along the way. so the above example should read:

QCString myData;
QCString myNewData = mangleData( myData );

QCString mangleData( const QCString& data )