Subject: Re: some comments about our C++ code...
From: Joaquín Cuenca Abela (cuenca@celium.net)
Date: Sun Apr 15 2001 - 19:47:02 CDT
Martin Sevior wrote:
>
> On Sun, 15 Apr 2001, [iso-8859-1] Joaquín Cuenca Abela wrote:
> >
> > 3) Please, please, please: NO public/protected variables. Period. Try
> > to avoid public virtual members, too.
> > This one need a bit of collaboration from everybody. I will not go
> > through all the code and fix that. It should be fixed in an incremental
> > way with the help of everybody.
>
> HI Joaquin,
> OK I have been guilty of violating this rule in many
> dialogs.
Everybody is guilty of violate some rules... we just should try to
reduce the violations :)
> Mike Nordell has told me off for doing it but it seemed the best
> way to implement some things.
Mike always is right.
Mike always is right.
Mike always is right.
Mike always is right.
Mike always is right.
Mike always is right.
:-)
> Specifically the XP layer of a dialog seems the natural place to hold
> variables concerned with the XP stuff in a dialog. In some dialogs there
> are a lot of XP variables. It just seemed natural to put them into
> "protected" space where they are accessible directly from the platform
> code. Then rather than write a whole lot of get/set functions, and in the
> case of vectors this is even sillier, it seemed more natural to make
> these variables accessible to the sub-classes where they can be
> read/written directly.
>
> Specifically if you genuinely need to read and write a variable in a
> subclass why put an extra layer of complexity between what you need to do
> and what you're actually doing?
because what you need to do today may be different than what you need to
do tomorrow. Usually you don't need to read and write a variable, you
need to get information from a class or you need to mutate a class.
When you bind these needs to a variable, you're binding what should be
the API with the implementation. I agree that in the abi sources you
can find some ocurrences when this issue is almost only theoric (usually
when you actually NEED a get/set pair), but there are many many places
when we are using protected: and the author really wanted private: (it
seems that private: scared some people or something).
For instance, I've been working a bit in the menu stuff, and I've
changed a
EV_Menu_Label **m_labelTable;
by an
UT_Vector m_labelTable; // <EV_Menu_Label *>, of course
what it means? if the class have had a good design, it will be a piece
of cake, because I would not need to check if the inherited classes of
EV_Menu_LabelSet were actually using m_labelTable (in this case, it has
been a piece of cake anyway because EV_Menu_Label doesn't have any
inherited classes, but it's not the case with EV_Menu, for instance).
These that still think that having protected: or public: variables is a
good idea, please ask yourself: why are public:, protected: & private:
sections? why can I have all that in a public: section?
The trick here is that it seems handy to have something more visible
than it should be. When you're writting it you think: if I left it
protected instead of private I'm not hurting anybody and this way it can
be accessed by my inherited classes... maybe they will need it.
Now think when you should change your code (and you will change it,
don't worry), the latest "it hurts nobody if I left it as protected"
becomes a nightmare, because now instead of just changing the
implementation of your class, you should too check all your inherited
classes! (and of course, you run in bigger problems if you also need to
change the api of you class. That's why everybody says that it's more
important to get a right api than to get a right implementation.)
I will that, yes, there are some places where I think that public: vars
are ok (in pod's for instance). But we can approx. it by "never".
> Another example is the fp_PageSize class in PD_Document. It just makes
> sense to leave this as a publically accessible class. Otherwise one would
> have to effectively duplicate all the methods in this class within
> PD_Document to change anything about it.
Ah, this one is good example.
right now we have:
class PD_Document : public AD_Document
{
public:
PD_Document(XAP_App *pApp);
// ... [snip many cool stuff (maybe too many :)]
fp_PageSize m_docPageSize;
// ...
};
instead of something like:
class PD_Document : public AD_Document
{
public:
PD_Document(XAP_App *pApp);
// ... [snip many cool stuff (maybe too many :)]
const fp_PageSize &getPageSize() { return m_docPageSize; }
void setPageSize(const fp_PageSize &pPageSize) { m_docPageSize =
pPageSize; }
private:
fp_PageSize m_docPageSize;
// ...
};
so if we want to change the page size of a document, we only have to do:
pDoc->m_docPageSize.whatever();
and we have a pretty natural way to change the page size of a document.
So far, no problems. Now thinks that some crazy dude does some tests
and he says: "hey, wait a second!, we're spending 1/2 of our memory in
instances of m_docPageSize!! and all them have nearly the same content!,
we should optimize out it!"
now, some smart dude (that has read the Design Patterns book (*hint*
*hint*)) thinks:
"ok, now problem, I just have to implement flyweight(*) in the
fp_PageSize objects used by PD_Document"
(think of it like instead of have a gazillion of fp_PageSize objects
having the same thing, you have in some weird central pool only 1
fp_PageSize object, and a gazillion of fp_PageSize pointers)
So, the smart dude only have to pass from:
==================================================
+----------------------------+
| PD_Document 1 |
+----------------------------+
| 300 bytes of fp_PageSize |
+----------------------------+
+----------------------------+
| PD_Document 2 |
+----------------------------+
| 300 bytes of fp_PageSize |
+----------------------------+
+----------------------------+
| PD_Document 3 |
+----------------------------+
| 300 bytes of fp_PageSize |
+----------------------------+
+----------------------------+
| PD_Document 4 |
+----------------------------+
| 300 bytes of fp_PageSize |
+----------------------------+
4 * 300 = 1200 bytes
=========================================
to
=========================================
+----------------------------+
| PD_Document 1 |
+----------------------------+
| 4 bytes of fp_PageSize * |
+----------------------------+
+----------------------------+
| PD_Document 2 |
+----------------------------+
| 4 bytes of fp_PageSize * |
+----------------------------+
+----------------------------+
| PD_Document 3 |
+----------------------------+
| 4 bytes of fp_PageSize * |
+----------------------------+
+----------------------------+
| PD_Document 4 |
+----------------------------+
| 4 bytes of fp_PageSize * |
+----------------------------+
+----------------------------+
| WeirdCentralPool |
+----------------------------+
| 300 bytes of fp_PageSize |
+----------------------------+
4 * 4 + 300 = 316 bytes
====================================
So, the smart dude changes PD_Document to use a flyweight design pattern
and keep low memory usage and happy users, and so PD_Document becames
something like:
class PD_Document : public AD_Document
{
public:
PD_Document(XAP_App *pApp);
// ... [snip many cool stuff (maybe too many :)]
const fp_PageSize &getPageSize() { UT_ASSERT(m_docPageSize); return
*m_docPageSize; }
void setPageSize(const fp_PageSize &pPageSize) { m_docPageSize =
askWeirdCentralPoolFor(pPageSize); }
fp_PageSize * m_docPageSize;
// ...
};
ok, so far, we can be proud of ourselves. We found a bug (memory
consumition too high), we know the solution, and we apply it... except
that it will not work. Why? Because everybody is using
pDocument->m_docPageSize.whatever();
instead of
pDocument->getPageSize().whatever();
(and here a regexp will not work, because we have too pDoc->, m_doc.,
and with a bit of bad luck pUnrelatedClass->m_docPageSize :)
So, we're basically screwed, now we should walk through all the code
that uses PD_Document to check if it uses m_docPageSize and change it
because we don't wanted to add 2 additional lines at the beginning :)
It's a bit hard to try to explain all that stuff in a foreign language,
so if there are still some things that look weird, please ask
(this example is not very far from the reality. Maybe fp_PageSize
doesn't need of flyweight but I'm sure that there are some other classes
that need it (properties?)).
> One example that is rife throughout the Abi code is the gnome FE code
> where the main Window widget pointer is left in protected space where it
> can be accessed and set by the over-loaded _constructWindow methods.
yes, I know, I know... when I have the time I will change it... :)
> Motto: Some rules are made to be broken.
yeah, but we're breaking it all the time, and this one most of the time
should not be broken.
Cheers,
-- Joaquín Cuenca Abela cuenca@celium.net
This archive was generated by hypermail 2b25 : Sun Apr 15 2001 - 19:47:09 CDT