decibel

[David Faure]

On Sunday 17 February 2008, Tobias Hunger wrote:
> Decibel being in kdereviews was not yet officially announced,

I had a quick look....

decibel/include/Decibel/{errors.h,dbusnames.h,accountdata.h} contain static objects, bad idea 
(especially in header files! those objects get duplicated in every user of the file, don't they?), 
use a singleton with K_GLOBAL_STATIC, or use const char* for all this.

types.h uses Q_DECL_EXPORT, which is enough for unix, but not portable to Windows, you need a lib-specific export macro

qWarning() << QObject::tr("Failed to setup handler for SIGTERM!"); }
We don't usually translate qWarning messages. End users don't see such messages, and many 
translators are going to ask you what SIGTERM means :) If it's too technical for translators
it's definitely too technical for end users.

> * This library is disQObject::tributed in the hope that it will be useful,

Massive search/replace alert :)
   
> static const QString filename("~/.decibel_contact_data.ini"); 

No static qstrings. No dot files in $HOME if you can avoid it, please use KStandardDirs.

>    static const QString component_search_path("~/.local/share/Decibel/components;"
>                                               "/usr/local/share/Decibel/components;"
>                                               "/usr/share/Decibel/components;"
>                                               COMPONENT_SEARCH_DIR);

Same there.
Why isn't this in share/apps/Decibel? Then you could use something like KStandardDirs::locate("data","Decibel/components").

>                QString key;
>                QStringList keys = settings.allKeys();
>                foreach (key, keys)
Don't declare a local variable that could be used by mistake after the foreach loop; prefer a const & for performance,
and a const QStringList to avoid detaching.
==>
    const QStringList keys = settings.allKeys();
    foreach(const QString& key, keys)

Same for all similar constructs in the code.

Should Decibel follow the kdelibs coding styleguide? The placement of {} looks very unusual at least:
>            if (i == profile.size())
>            { profile.append(component); }
>            else
>            { profile[i] = component; }


>     if (d->profiles.contains(profile_name))
>    { return d->profiles[profile_name]; }
>    else
>    { return QList<Decibel::Component>(); }

Double lookup, prefer using an iterator in general, or in this case, a simple call to value() would do the job.

>        QString profile_name;
>        foreach (profile_name, profiles.keys())  // [where profiles is a QHash]
>       {
>            [...]
>            for (int i = 0; i < profiles[profile_name].size(); ++i)
>            {
>                settings.setArrayIndex(i);
>
>                // display_name:
>                settings.setValue(display_name_key,
>                                  profiles[profile_name][i].display_name);
>                // protocols:
>                settings.setValue(protocols_list_key,
>                                  profiles[profile_name][i].protocol_list.join(QString(';')));

The lookup profiles[profile_name] is done everywhere; first step would be to use a local variable instead.
Even better would be to iterate using an iterator instead of iterating over profiles.keys() (which has to
construct a list first, this can be really slow, as discussed here recently).

>    if (filename.startsWith('~'))
>    {
>        QString home(qgetenv("HOME"));
>        use_filename = home + filename.mid(1);
>    }

This code is duplicated everywhere. Make a helper method? Or use KShell from kdecore, I think it can do this.

>     app.setOrganizationName(Decibel::organisation_name);

Smells like a mix of US and UK english ;-) Should be Decibel::organization_name for consistency.

> in demos/simpleclient/mytextchannelhandler.cpp:
>    // use iterators since TextChannel::Message has no default constructor yet...

Well, wouldn't be a problem if using a const ref as the foreach variable.


OK. I wish I could comment on the overall design but that's much harder to do ;)
Didn't see anything particularly striking, which is probably a good sign.

-- 
David Faure, faure at kde.org, sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).