El Dilluns, 5 de novembre de 2012, a les 22:59:43, Martin Klapetek va escriure: > On Mon, Nov 5, 2012 at 7:52 PM, Pino Toscano <p...@kde.org> wrote: > > 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 > > The data classes are finally done - all of them now use d-pointers and none > are qobject; I've added the qobject-parsers as Kevin suggested, please give > it a look.
You sure? I still see a lots of d-pointers missing (AllEventsListJob, AllNotesListJob, etc). Also you have some classes without the export thingie, like AttendeeInfo or the facebookTimeToKDateTime function Cheers, Albert > > > - 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() > > All fixed. > > > - 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? > > Well this classes are/should be writeable only from the QJson parser (which > passes the QVariant* stuff) as it reflects the server-side object. To all > the rest of the library & co. it's just a read-only container, so there is > no need for setters taking the *Info objects as they will never be used. > I'm wondering if the API should better reflect this by eg. making the > setters private and making the qobject-parsers friend classes.... > > > - in UserInfo there's birthdayAsString() while in other classes methods > > like that are named fooString() > > > > - UserInfo has no getters for city and country > > Both fixed. > > > - in UserInfo picture is a QUrl but website a QString? > > Fixed all links to be QUrl everywhere. > > > - remember to remove the qjson includes from public headers > > Done.