Hi, Alle sabato 27 ottobre 2012, Martin Klapetek ha scritto: > I'd like to move libkfacebook, the foundation for akonadi-facebook > resource, into extragear. It's been in use for a while, lots of > distro ship it bundled with akonadi-facebook resource, which is now > becaming part of kdepim-runtime for 4.10 with libkfacebook as > optional compile-time dependency. > > I'd like to ask for a review of this library, currently in kdereview > - https://projects.kde.org/projects/kdereview/libkfacebook
Some notes: - as others have pointed out, public classes should be better use d-pointers; for the jobs hierarchy, you could use a shared d-pointer, see [1] for example - GetCommentsJob::commentCount() could need the const qualifier - LikeInfo::setCount() does not need const& for the int parameter - ListJobBase::numEntries() (and in subclasses) could be better named as entriesCount() - ListJobBase, instead of a bool, could use an enum for readability - in CommentInfo, NotificationInfo and PostInfo some methods return *Info objects (or QList of them) while their setters take QVariantMap/QVariantList; better just make them take the object too, to not rely on the implementation (i.e. the parsing done with qjson)? also some of those getters have a *Map() version too, maybe those should be removed for the same reason above? - in UserInfo there's birthdayAsString() while in other classes methods like that are named fooString() - UserInfo has no getters for city and country - in UserInfo picture is a QUrl but website a QString? - remember to remove the qjson includes from public headers [1] http://techbase.kde.org/Policies/Library_Code_Policy/Shared_D-Pointer_Example -- Pino Toscano
signature.asc
Description: This is a digitally signed message part.
