On Sat, Oct 27, 2012 at 6:05 PM, Kevin Krammer <[email protected]> wrote:
> I had a cursory look and found a couple of things. CCing the original > authors > so they can provide input. > > - The job classes should have a QObject *parent = 0 argument and pass > parent > on to KJob's constructor. > > - Some of the constructors that can be called with only one argument are > missing the explicit keyword. > Will fix. > > - FacebookJob(QString, QString) calls setCapabilities, FacebookJob(QString) > does not > > - Is the FacebookJob(QString) constructor really needed. It seems all jobs > need a path > This is used (only) from FacebookGetJob, assumingly for querying multiple items, where just the ids are specified and no path is needed. I'll see what can be done about this. > - All jobs to the same URL base setup (protocol, host, token). I am > wondering > if the base class could just have a KUrl member and initialize all the > common > stuff and derived classes just add to that. > Will check. > - the *Info classes seem to be normal data classes, IMHO they don't need > to be > QObjects but rather "value types" > This is for the parsing purposes - the library uses QJson parser/mapper, which automagically maps the received json data to qobjects, otherwise there would have to be manual parsing everywhere (and the facebook jsons are huge), which means more code, more error possibilities, more maintaining requirement and worse readability (compared to two lines QJson mapper). So I'd like to leave this one as is. > - The typdefs in eventinfo.h create global types with very generic names, > e.g. > Event > Good point, will add those into namespace. -- Martin Klapetek | KDE Developer
