Review request for KSecretsService components

[Albert Astals Cid]

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.

Albert

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