<br><br><div class="gmail_quote">On Tue, Sep 29, 2009 at 3:09 PM, Thomas Zander <span dir="ltr"><<a href="mailto:zander@kde.org">zander@kde.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im">On Tuesday 29. September 2009 13.56.40 Dmitry Kazakov wrote:<br>
> On Tue, Sep 29, 2009 at 2:28 PM, Thomas Zander <<a href="mailto:zander@kde.org">zander@kde.org</a>> wrote:<br>
> > On Tuesday 29. September 2009 13.15.54 Dmitry Kazakov wrote:<br>
> > > Argh! =(<br>
> > > You are beaking encapsulation! Isn't this a basic principle of<br>
> > > object-oriented programming?<br>
> > > There is an object (doc), and there is it's interface<br>
> > > (undoLastCommand). And no calls to private members! (undo stack is<br>
> > > private, right?) Why do<br>
> ><br>
> > you<br>
> ><br>
> > > use private members?<br>
> ><br>
> > The code is;<br>
> ><br>
> > m_doc->undoStack()->undo();<br>
> ><br>
> > undoStack is a public method, not a private member. There is no breaking<br>
> > of encapsulation in this code.<br>
><br>
> There is. The object, returned by undoStack(), *is* private.<br>
<br>
</div>As soon as the public assessor was added, it stopped being private </blockquote><div><br>It's just a workaround that hides broken logic of the object.<br> <br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
and thus<br>
all results of it becoming public have to be taken. Good and bad.<br></blockquote><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
The good thing is that you can use it and don't have to create new methods,<br></blockquote><div><br>Are you that lazy to create a couple of methods? And allow "spaghetti code" instead?<br> <br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
the bad is that KoDocuments public API states we use the QUndoStack, so we<br>
can't stop using that.<br></blockquote><div><br>Why not just derive KoDocument form QUndoStack? Like<br><br>class KoDocument : QUndoStack {<br>...<br>};<br><br>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.<br>
<br><br><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
I think its just fine to decide in our API that we use and will continue to use<br>
the QUndoStack (or inherited). So I don't share your concerns and as we are<br>
indeed not breaking encapsulation due to that prior decision,</blockquote><div><br>Please take a look at this:<br><a href="http://en.wikipedia.org/wiki/Object-oriented_programming#Fundamental_concepts_and_features">http://en.wikipedia.org/wiki/Object-oriented_programming#Fundamental_concepts_and_features</a><br>
<br>"Method - an object's abilities. In language, methods (sometimes referred to as "functions") are verbs. <code>Lassie</code>, being a <code>Dog</code>, has the ability to bark."<br><br>In our case, KoDocument do not have any ability "to UndoStack". It is not a verb even.<br>
<br>More than that, usually, return values of the functions are considered as "out-values". But the construction above uses them as an 'in-value'.<br><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
its fine to use<br>
the code snipped pasted above.<br></blockquote></div><br>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]<br>
<br><br>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?<br>
<br><br clear="all">[0] - <a href="http://mags.acm.org/communications/200905/?pg=48&pm=2">http://mags.acm.org/communications/200905/?pg=48&pm=2</a><br>-- <br>Dmitry Kazakov<br>