On Mon, Nov 5, 2012 at 7:52 PM, Pino Toscano <span dir="ltr"><<a href="mailto:pino@kde.org" target="_blank">pino@kde.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

<div><div class="h5">Some notes:<br></div></div>
<br>
- as others have pointed out, public classes should be better use<br>
d-pointers; for the jobs hierarchy, you could use a shared d-pointer,<br>
see [1] for example<br></blockquote><div><br></div><div>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.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
- GetCommentsJob::commentCount() could need the const qualifier<br>
<br>
- LikeInfo::setCount() does not need const& for the int parameter<br>
<br>
- ListJobBase::numEntries() (and in subclasses) could be better named<br>
as entriesCount()<br></blockquote><div><br></div><div>All fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">


<br>
- ListJobBase, instead of a bool, could use an enum for readability<br>
<br>
- in CommentInfo, NotificationInfo and PostInfo some methods return<br>
*Info objects (or QList of them) while their setters take<br>
QVariantMap/QVariantList; better just make them take the object too,<br>
to not rely on the implementation (i.e. the parsing done with qjson)?<br>
also some of those getters have a *Map() version too, maybe those<br>
should be removed for the same reason above?<br></blockquote><div><br></div><div>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....</div>

<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
- in UserInfo there's birthdayAsString() while in other classes methods<br>
like that are named fooString()<br>
<br>
- UserInfo has no getters for city and country<br></blockquote><div><br></div><div>Both fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">


<br>
- in UserInfo picture is a QUrl but website a QString?<br></blockquote><div><br></div><div>Fixed all links to be QUrl everywhere.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">


- remember to remove the qjson includes from public headers<br></blockquote><div><br></div><div>Done.</div><br class="">-- <br><div><span style="color:rgb(102,102,102)">Martin Klapetek | KDE Developer</span></div></div>
</div>