KMetaData API comments

[Simon Hausmann]

Hi,

I've just had a look at the kmetadata public API and I have a few suggestions 
regarding consistency and naming. Please consider them constructive 
criticism.

QString KMetaData::errorString(int code);

	I think it would be nice of this function would take an enum instead.

enum ErrorCode {
           ERROR_SUCCESS = 0,
           ERROR_COMMUNICATION,
           ERROR_INVALID_TYPE
};

	In Qt/KDE enum values usually have camel case names instead of all upper 
case. The no-error value is usually also called "NoError" instead of 
having "success" in the name. (NoFoo is the common pattern).

I suggest

enum ErrorCode {
    NoError,
    CommunicationError,
    InvalidTypeError
};

int Resource::sync()

	I guess this function should return a ErrorCode enum instead if a plain int.

Variant Resource::getProperty(const QString &uri) const;

	Getters usually don't have a get suffix in Qt/KDE. I suggest to just call 
it "property".

Resource::remove()

	The documentation explains that this doesn't actually delete the resource but 
only marks it for deletion. I suggest to call it markForDeletion().

Resource::revive()

	In line with markForDeletion() this function could be called undelete(). That 
I think makes the relation to markForDeletion() also clearer (it is not 
obvious that revive relates to remove IMHO).

Resource::isProperty(const QString &uri) const

	Since this function is very different from the other property API in this 
class I suggest to disambiguate it by choosing a different name, if it is 
useful at all for applications. Perhaps the one line that it substitutes is 
not worth the naming conflict? I suggest to remove it alltogether. The call
to allResourcesWithProperty is easier to read IMHO despite its length.

bool Resource::modified() const

	The getter for a "modified" property is usually called "isModified" in 
Qt/KDE. I suggest to add the "is" prefix.

bool Resource::inSync() const

	Since this relates to modified but explains the status towards the metadata 
store, how about calling it isModifiedInStore()? Or maybe isModifiedOnDisk() 
would be more illustrative. (of course that would also reverse the meaning, 
but I think it's worth it).

QStringList Resource::getSymbols() const
quint32 Resource::getRating() const
QStringList Resource::getIdentifiers() const
QStringList Resource::getPrefLabels() const
QStringList Resource::getLabels() const
QList<Resource> Resource::getAnnotations() const
QString Resource::getComment() const
QList<Tag> Resource::getTags() const
QList<Resource> Resource::getTopics() const

	I think all of these getter functions should not have the "get" prefix.

QList<Resource> Resource::getIsRelateds() const;
QList<Resource> Resource::isTopicOfOf() const;

	Those feels like very very unusual names.

Since all of these getters are just convenience to prevent developers from 
having to write those long uris how about solving this problem using an enum 
instead? That would also reduce the public API dramatically and makes it 
really cheap to extend in the future. Right now when you want to add a few 
property you have to add two more exported symbols while an added enum does 
not affect the number of exported symbols.

I'm thinking of something along the lines of this:

Variant property(const QString &uri) const;
void setProperty(const QString &uri, const Variant &value);

enum PropertyType {
    SymbolsProperty,
    RatingProperty,
    IdentifiersProperty,
    IsRelatedProperty,
    IsTopicOfProperty,
    ...
};
Variant property(PropertyType type) const;
void setProperty(PropertyType type, const Variant &value);

On top of that such an enum based API is also easier to bind from other 
languages because not every setter/getter needs to be bound individually.

Perhaps this would also make it possible to get rid of the Tag class?

The only other thing in the API that *seriously* worries me is the Variant 
class that duplicates the *entire* QVariant API and registers a whole lot of 
types with the QMetaType registry.

Wouldn't it be simpler to solve the convenience problem this class tries to 
solve in the Resource API? You could have overloads for example:

QVariant property(const QString &uri) const;
QList<Resource> resourceListProperty(const QString &uri) const;

We do the same in Qt with QTextFormat for example, so it's not an entirely new 
pattern, and I think it is much better than duplicating the entire QVariant 
API, which is just asking for maintenance trouble IMHO.

Again, please consider this constructive criticism. I've only mentioned the 
bits that look inconsistent to me. There are indeed a lot of names that IMHO 
are very well chosen, for example ResourceManager::allResourcesOfType to 
name :) one.


Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20070501/b3a5f167/attachment.sig>