Jump to content
  • Advertisement

Archived

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

AdmiralBinary

"Smart" Constructor

This topic is 6024 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
Advertisement
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

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!