a question on class design

[jase jennings]

Hi,

I'm developing a suite of classes for downloading and parsing scripts from a web site. The scripts are of my own implementation and describe downloadable files from my site.

I've created a base class called CWebScript. This class knows how to download a script when given a URL. It also defines a pure virtual Parse() function. Inherited classes will define the Parse() implementation, as it is specific to the type of script being downloaded.

My issue is that i want the script class to to automatically download and parse the script when it is constructed. I want this implementation in the base class, so that inheriting classes get this functionality at no extra programming cost. Because Pasre() is pure virtual in the base class, i need the base class constructor to call the appropriate Parse() function in it's parent class. Is this a good way of going about it, or am i thinking along the wrong lines ?

Here's my classes :


const byte IWS_FAILED = 0; // default (non-specific) failure
const byte IWS_SUCCESS = 1; // success
const byte IWS_NULLSCRIPT = 2; // script is empty
const byte IWS_BADURL = 3; // url cannot be opened
const byte IWS_NOINET = 4; // no internet connection available
const byte IWS_URLNEEDSUPDATE = 5; // location of index script has moved. index script should be fetched again from new address
const byte IWS_NOSCRIPT = 6; // script cannot be opened

#define MAX_SCRIPT_SIZE 4096 // size of internal script buffer

class CWebScript // not exported from dll. this is a base class
{
public:

void SetScriptState(byte byteState) { m_byteState = byteState; }
byte GetScriptState() { return m_byteState; }

virtual byte Parse() = 0; // pure virtual function requires implementation in parent classes
byte Download(); // downloads a script into the internal script buffer

CWebScript(LPCTSTR lpScriptURL, byte byteAutodial = IC_ENABLEAUTODIAL);
virtual ~CWebScript();

protected:

byte m_byteState; // holds operational state. query for state of last function call using Get/Set methods

CInternetConnexion m_InetConnection;
CStringArray m_sParsedKeysArray;

bool m_bScriptDownloaded;
byte m_byteAutoDial;
CString m_sScriptURL;
CString m_sScriptBuffer;
};

// here's the implementatiuon of that constructor

CWebScript::CWebScript(LPCTSTR lpScriptURL, byte byteAutodial /* = IC_ENABLEAUTODIAL */)
{
m_sScriptURL = lpScriptURL;
m_bScriptDownloaded = false;

if(byteAutodial != IC_ENABLEAUTODIAL && byteAutodial != IC_DISABLEAUTODIAL)
{
byteAutodial = IC_ENABLEAUTODIAL;
}

m_byteAutoDial = byteAutodial;

if(lpScriptURL != NULL)
{
m_InetConnection.SetURL(lpScriptURL);
}

if((m_byteState = this->Download()) == IWS_SUCCESS) // Download() uses m_InetConnection to determine whether the url can be opened amongst other things
{
m_byteState = Parse(); // i can't do it like this obviously. is there a way ?
}

SetScriptState(m_byteState);
}





and here's an implementation of CWebScript, called CUrlScript. The script that it deals with contains urls to other scripts, and the Parse() function will contain the appropriate rules for parsing these urls from the script


class CUrlScript : public CWebScript
{
public:
CUrlScript(LPCTSTR lpScriptURL, byte byteAutodial = IC_ENABLEAUTODIAL);
virtual ~CUrlScript();

byte Parse();

};

// the constructor for CUrlScript constructs the CWebScript like this ...

CUrlScript::CUrlScript(LPCTSTR lpScriptURL, byte byteAutodial /* = IC_ENABLEAUTODIAL */) : CWebScript(lpScriptURL, byteAutodial)
{
// effectively, all construction is handled by the base class
}




I'd appreciate some advice please. can i call CUrlScript::Download() from the CWebScript constructor ? oibviously with the way i have it at the moment, the linker fails to find Parse().

Should i place Parse() in the CUrlScript contructor instead ? I wanted all this code in the base constructor so that future classes i create only need worry about parsing it's base classes downloaded script.



Jase

http://www.slideshowdesktop.com
View your images and photos on your desktop with ease using SlideShow Desktop, the desktop wallpaper manager for Microsoft Windows.

[anavlepo]

You seem to be calling derived class "parents", which sounds quite odd to me.

But anyway, of course, as I think you know, you cannot call virtual functions from your constructor and expect to get the final implementation. One solution to this is to use two-stage construction, which MFC does in some places -- client has to (1) construct the object, and then (2) create it (tell it to finish construction). Ugly, but then you have the final vtable in place during step 2.

[jase jennings]

i meant derived class. I just couldn't think of the terminology :)

I've redesigned my classes now. My previous idea was messy.

Thanks


Jase

http://www.slideshowdesktop.com
View your images and photos on your desktop with ease using SlideShow Desktop, the desktop wallpaper manager for Microsoft Windows.

[Paul McKenzie]

To add to Anavelepo:

It is not a good design to have your constructors do so much work.

a) What if the Parse fails? How would you know? You did create an object, but it is now unstable because some part of the contructor failed.

b) What if you pass these objects or use them in such a way that the compiler has to create temporary copies of the object? The constructors for the temp versions have to be called, meaning the Parse has to be called. This results in your code slowing down to a crawl if it is used in any relatively OO manner.

Constructors that do too much are *not* considered good class design for the reasons stated above. Can you imagine a constructor that opens a 1 megabyte file and reads it into a buffer, parses the file, etc.? Believe it or not, I've seen horribly written code that does this.

Constructors are meant for *simple* initializations, not for implementing whole sequence of operations that are expensive in terms of time or complexity. Stick with the model of doing this in two steps:

1) create a derived object, with the construtor just initializing simple member variables, so as to set the object in a valid "constructed" state. Maybe have a bool member that on construction is set to false, signifying that the Parse has not been initiated.

2) Then in a separate public function, the user of the class must call Parse() to actually start the parser. Set the bool member to true if Parse() is successful.

I once had to work on a problem where the programmer did almost exactly the same thing as what you are doing. The problem was that the constructor did so much work, but part of that work was failing. So an object was created, but it was unstable to use since the constructor failed. Don't fall into this trap.

Regards,

Paul McKenzie

[jase jennings]

Thanks Paul,

As ever, you make a good argument. I had already decided against this course of action, but not because i had considered the implications you describe. The download and parse take time, and rely on an internet connection that may or may not be there, and may or may not be fast. It's necessary for the client to display a progress dialog to make the reason for the time delay obvious to the user. For this reason, the client needs to construct, then download, then parse. These actions are enveloped by the display of a dialog.

My redesigned classes simply initialize the member variables of their respective classes now.




Jase

http://www.slideshowdesktop.com
View your images and photos on your desktop with ease using SlideShow Desktop, the desktop wallpaper manager for Microsoft Windows.