Archived

This topic is now archived and is closed to further replies.

"Smart" Constructor

This topic is 5741 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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!

Share this post


Link to post
Share on other sites
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

Share this post


Link to post
Share on other sites
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!

Share this post


Link to post
Share on other sites
(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!

Share this post


Link to post
Share on other sites
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

Share this post


Link to post
Share on other sites
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)();
}
};

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
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.

Share this post


Link to post
Share on other sites
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;


Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Go and look up the virtual constructor idiom. What you''re trying to do is well known, you''ve just done it wrong, that''s all.

Share this post


Link to post
Share on other sites
Thanx guyz
quote:
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.

Yeah, my second post actually states that I had to rewrite the code to get it to compile. I didn''t say how, though - this is what I did:

  
CMessage * msg = this;
delete msg;
msg = new CChatMessage;

And it worked...
quote:
What your are describing is similar to a "factory" pattern which is a perfectly acceptable OOP method.

OK, thanx - I thought of using a CNetConnection::GetMessage() and using it to return the right type of message, but at the time I thought it''d be "prettier" to do it the other way Oh well, we learn something new every day... Thanx, BTW - I''ll look into the factory thingy.
quote:
first off, did you get that to compile? i thought that modifying this was illegal.

Yeah, I had to change it to get it to compile
quote:
Hmmm... why arent you working on stick soldiers 2... someone deserves a beating...

Erm...well...yes - it''s coming along nicely (slowly), and we''re making great progress (we got the first few lines of code working) - we expect to have it finished real soon (look out for it in a coupla years).

Thanx again for your help everyone.

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

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

Share this post


Link to post
Share on other sites
Yep, you want a factory.

I have a CMessageFactory class that has three methods, IMessage* IMessage* CreateMessage(u8* pStream, u32 bytes)
bool RegisterMessage(IMessage*)
bool UnregisterMessage(IMessage*)
(and you probably don''t really need the UnregisterMessage one)

I create an empty message and use with the register call of the factory. IMessage is a reference counted interface, and has a Clone method. The factory maintains a map from the 32bit msgID to the registered IMessage*. When CreateMessage is called the factory sucks the msgID out of the stream, looks it up in the map, and uses found IMessage*''s Clone method to make a new message of the correct type. The new message then is responsible for deserializing itself from the reminder of the stream, where-upon it''s finally returned from the CreateMessage function.

It''s not perfect, but it works out reasonably well.

Share this post


Link to post
Share on other sites
Even better, plugable factories.

I think the article is at the site
"How Plugable Factories rock my multiplayer world" or something to that effect.

Has anyone else tried this? It seems to work pretty good.

Share this post


Link to post
Share on other sites
I read that article when I started working on my message system - it''s all good ideas, but it''s incomplete. IIRC, it doesn''t detail how you register messages at run-time, nor how the serialization/deserialization works.

You almost want a IBlob structure that reference-count manages network buffers - kinda how IMediaSample works in DirectShow. But you also want a (message) structure that makes sense of what''s in that blob.

Share this post


Link to post
Share on other sites
At load time, the static member causes them all to register. If I remember correctly, the sample code with the article is indeed incomplete, you just need to declare the statics to get it to compile.

As far as serialization goes, I wrote a to/fromWire function that spits out/read in a string and the length. It is all really basic code, so I wrote a quick PERL script that writes it all for me. Just a bunch of memcopies.

Share this post


Link to post
Share on other sites