Common Programming Mistakes

Anonymous namespaces vs statics

If you have a method in a class that does not access any members and therefore 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. Be careful: this notation is used in some codes for function parameters instead.

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.

Do not use class-static variables, especially not in libraries and loadable modules (but I would even discourage it in applications). Static objects lead to lots of problems (crashes) due to undefined order of construction/destruction. Instead, use a static pointer, together with KStaticDeleter.

Forward Declarations

You will reduce compile times by forward declaring classes when possible instead of including their respective headers. For example:

    #include <qwidget.h>     //bad
    #include <qstringlist.h> //bad
    #include <qstring.h>     //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<SomeClass> 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.

Delayed Initialization

Although the design of modern C++ applications can be very complex, one recurring problem that is generally easy to fix is not using the technique of delayed initialization, as described by KDE developer Zack Rusin. First of all, let's look at the standard way of initializing a KDE application:

    int main( int argc, char **argv )
    {
	....
	KApplication a;

	KCmdLineArgs *args = KCmdLineArgs::parsedArgs();

	MainWindow *window = new MainWindow( args );

	a.setMainWidget( window );
	window->show();

	return a.exec();
    }
    

Notice that window is created before the a.exec() call that starts the event loop. This implies that we want to avoid doing anything non-trivial in the top-level constructor, since it runs before we can even show the window.

The solution is simple: We need to delay the construction of anything besides the GUI until after the event loop has started. Here's how the example class MainWindow's constructor could look:

    MainWindow::MainWindow()
    {
	initGUI();
	QTimer::singleShot( 0, this, SLOT(initObject()) );
    }

    void MainWindow::initGUI()
    {
	/* Here's where you construct your widgets.  Note that the widgets you
	 * construct here shouldn't require complex initialization either, or
	 * you've defeated the purpose.
	 * All you want to do is create your GUI objects and QObject::connect
	 * the appropriate signals to their slots.
	 */
    }

    void MainWindow::initObject()
    {
	/* This slot will be called as soon as the event loop starts.  Here's
	 * where you could put everything else that needs to be done, including
	 * restoring values, reading files, session restoring, etc.  It will
	 * still take time, but at least your window will be on the screen,
	 * making your app look busy.
	 */
    }
    

Using this technique may not buy you any overall time, but it makes your app seem quicker to the user who is starting it, and is more reassuring to boot, as users don't have to click on an icon and then wonder if the app is really going to start or not.

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 inadvertently using QString along the way. so the above example should read:

    QCString myData;
    QCString myNewData = mangleData( myData );

    QCString mangleData( const QCString& data )
    

QDomElement

When parsing XML documents, one often needs to iterate over all the elements. You may be tempted to use the following code for that:

    for ( QDomElement e = baseElement.firstChild().toElement(); !e.isNull();
          e = e.nextSibling().toElement() )
    {
       ...
    }
    

That is not correct though, the above loop will stop prematurely when it encounters a QDomNode that is something other than an element. A comment for example. The correct loop looks like:

    for ( QDomNode n = baseElement.firstChild(); !n.isNull();
           n = n.nextSibling() )
    {
        QDomElement e = n.toElement();
        if (e.isNull()) continue;
        ...
    }
    

QObject

If you ever need to delete a QObject derived class from within one of its own methods, don't ever delete it via the:

   delete this;
   

idiom. This will sooner or later cause your application/library to crash because a method on that object might be invoked from the Qt event loop via slots/signals after you deleted it. Always use

   deleteLater();
   

method from QObject which tries to do the same thing as delete this in a safer way.