Review request for KSecretsService components
Albert Astals Cid
aacid at kde.org
Thu Nov 10 01:08:12 GMT 2011
A Dijous, 10 de novembre de 2011, Valentin Rusu vàreu escriure:
> On 11/10/2011 01:00 AM, Valentin Rusu wrote:
> > On 11/10/2011 12:48 AM, Albert Astals Cid wrote:
> >>>> * createItem falls in the "let's use a bool instead of an
> >>>> enum
> >>>> because it>
> >>>> just have two values" trap for the replace parameter
> >>> Well, I don't agree. When presenting a new item to the collection,
> >>> you
> >>> should specify to replace existing item or not. Yes/No is boolean
> >>> for me.
> >> That is the problem, that a boolean is Yes/No, and then you end up
> >> with a call
> >> that says
> >> foo->createItem(label, attributes, mySecret, false);
> >> And it seems you do not want to create the item :D while
> >> foo->createItem(label, attributes, mySecret, DoNotReplace);
> >> is much more obvious.
> >> More on my side at
> >> http://wiki.qt-project.org/API_Design_Principles#The_Boolean_Parameter
> >> _Trap
> >> and
> >> http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html
> > Ok, I see. I'll change that.
> Well, In fact I changed my mind, as the enum must be a member of
> Collection class, but required to be used in the create job and nested
> enums cannot be forward declared. So that'll require to move the enum
> outside of the Collection class, leading to even uglier code.
There is a solution for this, the CreateCollectionItemJob constructor should
accept a CreateCollectionItemJobPrivate instead of all the params (since i
understand it does not make sense for me creating one directly and I will only
get one thought Collection::createItem (reason to make it private too?)). This
way you do not need a replace member in the constructor since Collection
passes the CreateCollectionItemJobPrivate directly.
Probably the same argument for making the constructor private for all the
other jobs applies too.
> So I'll
> stay with the boolean, wich also corresponds to the freedesktop.org dbus
> interfaces spec, btw.
> Valentin Rusu (IRC valir, KDE vrusu)
> KSecretsService (former KSecretService, KWallet replacement)
More information about the kde-core-devel