Back to General and Gameplay Programming

"Smart" Constructor

General and Gameplay Programming Programming

Started by AdmiralBinary March 23, 2002 02:03 AM

15 comments, last by AdmiralBinary 22 years ago

AdmiralBinary

100

Author

March 23, 2002 02:03 AM

I''m writing a simple chat application, and I''m implementing a CMessage class. CMessage will be a base class (not an ADT, but still acting like one), with children like CChatMessage, CBootMessage, or CDrawMessage. Now, in order to keep it as OO as possible, I thought of having a "smart" constructor for CMessage which would allow me to say: new CMessage(netConnection), and the object would actually be the right kind of CMessage. I don''t think I''m explaining well - maybe this will:

  
//Grabs a message from the socket queue.

CMessage * msg = new CMessage(netConnection);
msg->Process();

  
CMessage::CMessage(CConnection * con)
{
 BYTE messageType;
 con->Read(sizeof(BYTE),&messageType);
 switch(messageType)
 {
  case CHAT_MESSAGE:
   delete this;
   //Would have to implement another constructor for CChatMessage to avoid this one being called recursively.

   this = new CChatMessage(netConnection);
   break;
   case BOOT_MESSAGE:
   //....

   break;
 }
}

My question is this: Is this a gross misuse of the language, or acceptable OO programming practice? I''m guessing that it''ll be the former, but I''m just checking... BTW, I have tried this technique, and it works. I just dunno if it''s safe etc. --------------- #define TRUE 0 #define FALSE 1 //MUAHAHAHAHAAAA!

TheMuuj

122

March 23, 2002 02:18 AM

It is a gross misuse of OOP principles (I''m not even sure it will have the same behavior on all compilers), but that''s not my biggest problem with it.

My problem is that your constructor isn''t truthful. I mean, if I create an object of one type with a constructor, I expect to get an object of that type (not of any sub-type).

If you insist on doing something like this, then make the constructor of the base class protected or private, and make the constructors of the derived classes private, but then make the base class a friend of the derived class.

Then make a static function in your base class called Create() or Get() (or something like that), and have it return the correct type of object.

But this also bothers me because I don''t like to make my base classes aware of every derived class. What if you wanted to add another derived class...you would have to modify the base class, thus defeating the point of inheritance.

So your creation function should probably go in the class that handles your network connection. If that isn''t your class, you should write a wrapper class. Then don''t worry about making a weird constructor, just use this syntax

CMessage* msg = netConnection->ReadMessage();

This makes much more sense to me, even if it means making your message constructors private and making yoru conenction a friend of your messages.

Not to mention it isn''t always a good idea to use the this pointer within your constructor (but there are cases where you can).

--TheMuuj

--TheMuuj

AdmiralBinary

100

Author

March 23, 2002 02:20 AM

OK, I just discovered I''ll have to use a slightly different method to get this to work (I''ll have to declare a variable pointer to this and then call delete and new on that), but the question remains the same.

---------------

#define TRUE 0
#define FALSE 1
//MUAHAHAHAHAAAA!

AdmiralBinary

100

Author

March 23, 2002 02:23 AM

(Posted above before seeing your message, TheMuuj.)
Thanx very much - I shall go and do penance for even thinking of committing such an atrocity

Static function hadn''t even crossed my mind. I''ll definitely be implementing that. Once again, thanx

---------------

#define TRUE 0
#define FALSE 1
//MUAHAHAHAHAAAA!

TheMuuj

122

March 23, 2002 02:38 AM

Don''t be so hard on yourself. You have a lot going for yourself.

1) At least you asked. People on these message boards are always willing to dictate the correct principles of OOP (whether you want them to or not). At least I answered you before someone started a flame war.

2) You were willing to make a change. Your changes may not result in perfect OOP code, but you have probably taken a step in the right direction. A lot of people with huge flaws in their code are unwilling to change it. I know this is true even for me. My code is gold, and nobody can touch it. :-P

3) You got the flawed code to work. (not a great argument, but hey, I gotta respect you for it)

--TheMuuj

--TheMuuj

EvilCrap

134

March 23, 2002 03:58 AM

first off, did you get that to compile? i thought that modifying this was illegal.

why are you using polymorphism, is it really needed? how do you determine what to send to your creation function?

instead of having static functions in your base class determine what to do, you might want to put everthing in a namespace or a helper class.

do you really need to use child classes? do you just want to model modes?

  class Cmessage{         //current mode	 void (Cmessage::*_mode)();	 //modes	 void prompt(){ cout << "prompt";}public:	Cmessage(){}	Cmessage(int mode) : _mode(NULL)	 {		switchMode(mode);	 }	 void switchMode(int mode)	 {	  switch(mode)	   {		case 0:		_mode = prompt;	   }	 }	 void process(){	   //you might find a switch more logical	   (this->*Cmessage::_mode)();	 }};

elis-cool

271

March 23, 2002 05:23 AM

Hmmm... why arent you working on stick soldiers 2... someone deserves a beating...

[email=esheppard@gmail.com]esheppard@gmail.com[/email]

Anonymous

March 23, 2002 09:14 AM

Try to avoid using "new", you certainly don''t need it here. C++ has automatic objects which destroy themselves when they go out of scope. If you use it all the time your opening up a world of hell in terms of memory leaks. Even if you use smart pointers and things, try to avoid calling new explicitally; there is no need.

MetalicFog

122

March 23, 2002 10:19 AM

What your are describing is similar to a "factory" pattern which is a perfectly acceptable OOP method.

  class CMessage;class CChatMessage : public CMessage;etc ...class MessageFactory{public:   static CMessage* Create(BYTE msg_type)   {      switch(type)      {         case CHAT_MESSAGE : return new CChatMessage;         etc ...      }   }};Cmessage* msg = MessageFactory::Create(netConnection.MsgType());msg->Process();delete msg;

Fruny

1,658

March 24, 2002 04:37 AM

Just commenting on the first snippet of code posted :

this = new CChatMessage(netConnection);

The fact that you are assigning to the this pointer will not affect the object you have constructed (except for whatever member functions that you might call later in the constructor, to which this is transparently passed).

If this ever compiles, all you''ll get is a memory leak.

Listen to MetallicFog. What you want is a factory.

"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." — Brian W. Kernighan

"Smart" Constructor

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

"Smart" Constructor

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines