koffice

[Dmitry Kazakov]

On Tue, Sep 29, 2009 at 3:09 PM, Thomas Zander <zander at kde.org> wrote:

> On Tuesday 29. September 2009 13.56.40 Dmitry Kazakov wrote:
> > On Tue, Sep 29, 2009 at 2:28 PM, Thomas Zander <zander at kde.org> wrote:
> > > On Tuesday 29. September 2009 13.15.54 Dmitry Kazakov wrote:
> > > > Argh! =(
> > > > You are beaking encapsulation! Isn't this a basic principle of
> > > > object-oriented programming?
> > > > There is an object (doc), and there is it's interface
> > > > (undoLastCommand). And no calls to private members! (undo stack is
> > > > private, right?) Why do
> > >
> > > you
> > >
> > > > use private members?
> > >
> > > The code is;
> > >
> > > m_doc->undoStack()->undo();
> > >
> > > undoStack is a public method, not a private member. There is no
> breaking
> > > of encapsulation in this code.
> >
> > There is. The object, returned by undoStack(), *is* private.
>
> As soon as the public assessor was added, it stopped being private


It's just a workaround that hides broken logic of the object.


and thus
> all results of it becoming public have to be taken. Good and bad.
>

> The good thing is that you can use it and don't have to create new methods,
>

Are you that lazy to create a couple of methods? And allow "spaghetti code"
instead?


the bad is that KoDocuments public API states we use the QUndoStack, so we
> can't stop using that.
>

Why not just derive KoDocument form QUndoStack? Like

class KoDocument : QUndoStack {
...
};

It would make an api clearer and more maintainable. Example, if you decide
to change undo() method one day (e.g. add locking), you will just need to
overload one virtual method instead of overloading the whole QUndoStack and
changing all appropriate constructors in KoDocument.





>
> I think its just fine to decide in our API that we use and will continue to
> use
> the QUndoStack (or inherited). So I don't share your concerns and as we are
> indeed not breaking encapsulation due to that prior decision,


Please take a look at this:
http://en.wikipedia.org/wiki/Object-oriented_programming#Fundamental_concepts_and_features

"Method - an object's abilities. In language, methods (sometimes referred to
as "functions") are verbs. Lassie, being a Dog, has the ability to bark."

In our case, KoDocument do not have any ability "to UndoStack". It is not a
verb even.

More than that, usually, return values of the functions are considered as
"out-values". But the construction above uses them as an 'in-value'.



> its fine to use
> the code snipped pasted above.
>

Well, zooming subsystem has already become unusable because of the access to
private members (managers have cyclic links to each other). That's why it
has hacks now. Do you really want the same fate for KoDocument? "API design
matters" [0]


Btw, if you still insist on your method, please answer a question: how are
you going to add locking for undo() operations in the future? For example,
one day you will need to acquire "BigSomeStuffLock" on every undo. How are
you going to do this?


[0] - http://mags.acm.org/communications/200905/?pg=48&pm=2
-- 
Dmitry Kazakov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kimageshop/attachments/20090929/0296b8ad/attachment.htm