Subject: some comments about our C++ code...
From: Joaquín Cuenca Abela (cuenca@celium.net)
Date: Sun Apr 15 2001 - 10:16:31 CDT
1) Please, please, please, the destructors should *ALWAYS* be virtual
(except if you want to indicate that nobody should inherit from your
class).
Somebody forget to virtualize the EV_Menu destructor, and so we were
(are) leaking all the resources of EV_Menu's littles (EV_UnixMenu,
EV_WinMenu, EV_UnixGnomeMenu, etc, etc.)
If you think that it's not so important, because anyway the menus are
only destroyed at the end of the application, you're forgetting about:
* Closing frames.
* Pop-up menus.
2) There is no point in a "singleton" that has a public "*instance"
member, (btw, it doesn't have even a get_instance, and its constructor
is not protected). I'm speaking of XAP_EncodingManager. I will not
list all the flaws that it has (for instance, try:
new XAP_EncodingManager();
...
XAP_EncodingManager *imasingleton = XAP_EncodingManager::instance;
...
new XAP_EncodingManager();
..
XAP_EncodingManager *really = XAP_EncodingManager::instance;
UT_ASSERT(imasingleton == really); // of course, here we assert.
...
// I'm evil
delete XAP_EncodingManager::instance;
...
imasingleton->ohMyGod();
well, you've got the point).
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.
4) Please, be nice and try to keep the .h down to a minimum. That
includes
* Minimum number of #includes. Use class Toto; instead or #include
"toto.h" if you're only using a ref or pointer of type Toto in your .h
* Keep the big comments in the .cpp
5) Don't use static buffers. Specially if they have a size of 8000
(!!!) bytes (yes, 4000 and 2000 are too, too much).
Cheers,
P.S.: Please, don't fix EV_Menu nor XAP_EncodingManager. I've already
changed the code locally, but it will take a look until I can commit.
-- Joaquín Cuenca Abela cuenca@celium.net
This archive was generated by hypermail 2b25 : Sun Apr 15 2001 - 10:16:50 CDT