Help with rule of 3 pleae.

[Jim Langston]

I understand the rule of three, that if I have a custom constructor, copy or
destructor I probably need the other 2. My class object definately has a
custom constructor and destructor, but I'm fuzzy on copy.

Could someone help me out with this class? Shown are the variables,
constructors and destructor only (rest snipped out to preserve space). Yes,
I realize this is fairly ugly code, any suggestions/critiques on that would
be appreciated also. I know some won't like the fact it's called
CGUIElement and not GUIElement.

I guess my main question is what to do about the TEXTURE* (all caps is from
library I'm using, not my idea) and how to handle them if it's copied. In
theory I guess I would only like to accept a shallow copy with the
confirment that the original is not used anymore.

Currently they are being created like this:
Client.GUIElements[Client.GUI_BITMAPS] =
new CGUIElement( ClosedFileName, jColor(255, 0, 240, 255), GUI_List,
726, 674,
FileNames, 85, 44 );

and I can't think of anywhere they would actually be copied, but am not
sure.

How would I go about disabling copies so that this class could never be
copied since I don't have a copy override?

Any advice is appreciated.

// Header //
class CGUIElement
{
private:
TEXTURE* OpenBitmap; // Bitmap to show when GUI is "Open"
TEXTURE* ClosedBitmap; // Show when GUI is not "Open" (or ExtraBitmaps)
std::vector<TEXTURE*> ExtraBitmaps; // Used when more than 2 bitmaps are
needed
GUIType Type; // Kind of element this is
int ListIndex; // If type List what bitmap to show when open?
int NotifyMsg; // Some Observer has requested we send this info
// on Notifies
unsigned long BitmapX, // Where to display this bitmap
BitmapY;
unsigned int Width, // Size of bitmap
Height;
unsigned int X1, Y1, X2, Y2; // Used for clickable selection
unsigned int TextX, // Just to store/return text x/y
TextY,
TextX2,
TextY2,
TextWidth, // Window Max Text Length
TextHeight; // Window Max Text Lines
bool IsLoaded; // Will normall be true, unless error loading
bool IsOpen; // Is the attached window, or this window "Open"?
public:
CGUIElement( std::string OpenFileName, std::string ClosedFileName,
JCOLOR ColorKey,
GUIType WhatType,
unsigned long X, unsigned long Y,
unsigned int ThisX1 = 0, unsigned int ThisY1 = 0,
unsigned int ThisX2 = 0, unsigned int ThisY2 = 0,
unsigned int textX = 0, unsigned int textY = 0,
unsigned int textX2 = 0, unsigned int textY2 = 0);
virtual ~CGUIElement();
};

// CPP file //
CGUIElement::CGUIElement( std::string OpenFileName, std::string
ClosedFileName,
JCOLOR ColorKey,
GUIType WhatType,
unsigned long X, unsigned long Y,
unsigned int inX1, unsigned int inY1,
unsigned int inX2, unsigned int inY2,
unsigned int textX, unsigned int textY,
unsigned int textX2, unsigned int textY2) : OpenNotifier(this),
OpenObserver(this),
CloseNotifier(this), CloseObserver(this),
CloseOnOpenNotifier(this), CloseOnOpenObserver(this),
IsOpen(false), IsLoaded(false),
NotifyMsg(0),
Type(WhatType),
BitmapX(X), BitmapY(Y),
X1(inX1), Y1(inY1),
X2(inX2), Y2(inY2),
TextX(textX), TextY(textY),
TextX2(textX2), TextY2(textY2),
TextWidth(0), TextHeight(0)
{
OpenBitmap = (TEXTURE*) NULL;
ClosedBitmap = (TEXTURE*) NULL;
OpenBitmap = new TEXTURE;
jLoad_Texture(OpenBitmap, const_cast<char*>( OpenFileName.c_str() ),
false, ColorKey, 0);
if ( ! OpenBitmap->Success )
{
IsLoaded = false;
delete OpenBitmap;
OpenBitmap = (TEXTURE*) NULL;
ClosedBitmap = (TEXTURE*) NULL;
}
else if ( ClosedFileName.length() > 0)
{
ClosedBitmap = new TEXTURE;
jLoad_Texture(ClosedBitmap, const_cast<char*>(
ClosedFileName.c_str() ), false, ColorKey, 0);
if ( ! ClosedBitmap->Success )
{
// Couldn't load 2nd bitmap, unload 1st
jRelease_Texture(OpenBitmap);
delete OpenBitmap;
delete ClosedBitmap;
ClosedBitmap = (TEXTURE*) NULL;
OpenBitmap = (TEXTURE*) NULL;
IsLoaded = false;
}
else
IsLoaded = true;
}
else
{
ClosedBitmap = (TEXTURE*) NULL;
IsLoaded = true;
};
if ( IsLoaded )
{
Width = OpenBitmap->Width;
Height = OpenBitmap->Height;
}
};

CGUIElement::~CGUIElement( )
{
if ( OpenBitmap != NULL )
{
jRelease_Texture(OpenBitmap);
delete OpenBitmap;
}
if ( ClosedBitmap != NULL )
{
jRelease_Texture(ClosedBitmap);
delete ClosedBitmap;
}
std::vector<TEXTURE*>::iterator it;
for ( it = ExtraBitmaps.begin(); it != ExtraBitmaps.end(); ++it )
{
TEXTURE* Bitmap = (*it);
jRelease_Texture(Bitmap);
delete Bitmap;
}
};

 

[Jim Langston]

Currently they are being created like this:
Client.GUIElements[Client.GUI_BITMAPS] =
new CGUIElement( ClosedFileName, jColor(255, 0, 240, 255), GUI_List,
726, 674,
FileNames, 85, 44 );

Actual call is:
Client.GUIElements[Client.GUI_BITMAPS] =
new CGUIElement( OpenFileName, ClosedFileName, jColor(255, 0, 240, 255),
GUI_Static,
0, 674);

(other sample was from the *other* constructor I'm not showing to preserve
space)

 

[Alf P. Steinbach]

* Jim Langston:

I understand the rule of three, that if I have a custom constructor, copy or
destructor I probably need the other 2. My class object definately has a
custom constructor and destructor, but I'm fuzzy on copy.

Copying can be done by two special member functions: the copy constructor, and
the assignment operator.

struct T
{
T( T const& another ) { ... } // Copy constructor.
T& operator=( T const& newValue ) { ... } // Assignment.
};

Could someone help me out with this class? Shown are the variables,
constructors and destructor only (rest snipped out to preserve space). Yes,
I realize this is fairly ugly code, any suggestions/critiques on that would
be appreciated also.

My first instinct would be to reduce the number of variables, by e.g. using
some Point or Rectangle class first of all.

However, after looking a bit further down in your post I see something much
more important: that your constructor sets an error indication 'IsLoaded'
instead of throwing an exception. Which means your destructor gets
complicated, and all other member functions. Make the loadedness of the
bitmaps a class invariant (absolute requirement), and throw if not, and as
part of that, start using std::auto_ptr for automatic clean-up.

Then, reduce the number of variables, etc.

I know some won't like the fact it's called
CGUIElement and not GUIElement.

I guess my main question is what to do about the TEXTURE* (all caps is from
library I'm using, not my idea) and how to handle them if it's copied. In
theory I guess I would only like to accept a shallow copy

That's what you have now: only the pointers, not the bitmaps, are copied.

with the
confirment that the original is not used anymore.

That, you don't have...

How would I go about disabling copies so that this class could never be
copied since I don't have a copy override?

Declare a private copy constructor and assignment operator. You don't have
to implement them.

 

[Jim Langston]

My first instinct would be to reduce the number of variables, by e.g.
using
some Point or Rectangle class first of all.

Since almost all the X,Y values are pairs, not quads, and define a point,
not
a square, I guess I could go with a point. This seems to reduce the number
of variables by a bit since I seem to have a lot of pairs.

When I started I only had one set, the top x/y coords. But as I kept adding
functionality to the class they just kept piling up. Okay, thanks, I think
I'll definately use Point and figure out how to pass a hard coded structure
as a parameter. Guess I have to make a constructor for the structure taking
2 parms and build it then pass it?

However, after looking a bit further down in your post I see something
much
more important: that your constructor sets an error indication 'IsLoaded'
instead of throwing an exception. Which means your destructor gets
complicated, and all other member functions. Make the loadedness of the
bitmaps a class invariant (absolute requirement), and throw if not, and as
part of that, start using std::auto_ptr for automatic clean-up.

Orignally there wasn't that much error checking to see if it was actually
loaded, and what would happen is when I was building my window system (this
is graphical GUI system for game I'm writing) I would misname a bitmap and
when the program was run I would get a hard crash when the program went to
display the graphic.

I was originally going to halt the program if it couldnt' find a graphic,
but I was adding so many at once (10+) that then I had to find one problem,
fix it, recompile, run, fix it, etc... so I came up with the IsLoaded, so
that if it didn't find the graphic it would just return a NULL pointer for
the image (which my game was already checking for anyway) so I could test
more than one at a time.

Since it's already implemented it seems to me that it's nicer for the
program
to simply not display a graphic image if it can't find it then to hard
crash. Or if not hard crash then I have to do try...except blocks in main
program which gets messy.

I don't think that with the nature of this class it should be an absolute
requirement that it find the image, especially since this class turns out
to be so useful and reusable I'm not sure what I'll be using it for next.
(It took me maybe 1/2 hour when I changed engine from DirectX to another
one to change the class to use the new engine).

That's what you have now: only the pointers, not the bitmaps, are copied.

Understood. Which is bad with the heavy use of pointers and new in the
class.

That, you don't have...

Right. Not sure if I can. But your next suggestion seems to fit the
bill...

Declare a private copy constructor and assignment operator. You don't
have
to implement them.

This is an excellant idea!

This also means that in the future for any class I make with a custom
constructor
using pointers I'm going to add private copy and assignment methods so that
if
any program does try to use them before I build them they won't compile.

Excellent feedback, Thanks!

 

[Jim Langston]

Declare a private copy constructor and assignment operator. You don't
have
to implement them.

// Header //
class CGUIElement
{
private:
// No copy or assignment yet so disable by making private.
CGUIElement ( CGUIElement const& CopyThis ): OpenNotifier(NULL),
OpenObserver(NULL),
CloseNotifier(NULL),
CloseObserver(NULL),
CloseOnOpenNotifier(NULL),
CloseOnOpenObserver(NULL)
{ assert ( 1 == 2 ); }; // Copy constructor.
CGUIElement& operator=( CGUIElement const& CopyThis )
{ assert ( 1 == 2 ); }; // Assignment.
....
};

Definately not doing any shallow copies then, since program compiles and
runs.

Thanks again.