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)