[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Moving libkfacebook to extragear
From:       Martin Klapetek <martin.klapetek () gmail ! com>
Date:       2012-11-05 21:59:43
Message-ID: CAPLgePqC=zF1-U+S1BZJS09Gm-jvTYubjR1wRcAQ+04Totwhiw () mail ! gmail ! com
[Download RAW message or body]

On Mon, Nov 5, 2012 at 7:52 PM, Pino Toscano <pino@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.


> - 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.

-- 
Martin Klapetek | KDE Developer

[Attachment #3 (text/html)]

On Mon, Nov 5, 2012 at 7:52 PM, Pino Toscano <span dir="ltr">&lt;<a href="mailto:pino@kde.org" \
target="_blank">pino@kde.org</a>&gt;</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&#39;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&amp; 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 &amp; co. it&#39;s just a read-only container, so \
there is no need for setters taking the *Info objects as they will never be used. I&#39;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&#39;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>



[prev in list] [next in list] [prev in thread] [next in thread]