Is it a good practice to delete an object in it's constructor? For example, my TGATextureData class uses a constructor to load it's data. It takes in a targa filename, and if my parser supports the format, then it'll load. Now, if this file isn't valid, isn't actually a targa file, or isn't in the format I support, I make my constructor return early. The problem is, I want my constructor to return my object as NULL.
Here's an example:
class TGATextureData
{
protected:
bool loaded;
TGATextureData::TGATextureData(const char *filename)
{
Initialize(); // initialize all members
FILE *fp = fopen(filename, "rb");
if(!fp)
{
printf("TGATextureData::TGATextureData(%s): invalid file\n", filename);
// delete this; // not a good idea...
return; // cancel out early
}
// continue loading the data from the file
loaded = true;
}
...
TGATexture *tgaData = new TGATextureData("texture.tga"); // texture.tga does not exist!
The code there returns if the supplied filename cannot be opened, or does not exist. What I'd like to do is have my "new TGATextureData('texture.tga')" call return NULL, and not allocate anything at all if something's not correct, but is that possible?
I have a solution to this, but it's a C-based approach. It involves creating a wrapper function outside of the class which tests if construction failed or not:
TGATextureData *LoadTGATextureData(const char *filename)
{
TGATextureData *tgaData = new TGATextureData(filename);
if(!tgaData->IsLoaded())
{
delete tgaData;
tgaData = NULL;
}
return tgaData;
}
What that does is load the data to a temporary pointer in the stack. Once constructed, the "loaded" flag is tested if it's loaded or not. If loaded, the pointer to the allocated data is returned as output, or if it's not properly loaded, the object is released from memory, and a NULL pointer is passed. Does this approach seem like good practice for a C++ programmer, or are there better ways of doing this?
Thats constructors for you.
It allocates the memory and runs whatever code for intiialisation. returning null is impossible.
What you usually do is:
a) use the factory method (you used it on the second part)
or
b) throw an exception during the constructor. Use a three stage build for this: constructor intiialises the entire object but can fail and call terminate and then throw. destructor calls terminate. Terminate is called whenever you want to destroy the object (FROM the destructor! dont make this one public!).
Its very important that you deallocate anythng you might allocate during the constructor before you throw yourself out of the constructor! If you dont you will end up with mem leaks.
Does anyone else have a better process than this? I dont particularly fancy the try catch penalty that this requires but I never saw a better solution (other than factory methods).
Oh... yeah.. forgot c)
c) make your constructors fail safe! IE: pre process any resources required for them before hand and pass them directly to the constructor... that way the constructor never fails and there isnt anything that can fail.
But c) is just moving the internal code of the constructor outside and testing it before doing the creation... not really a solution.
What you are trying to do with a constructor is against how c++ defines constructors. Your solution is to use the constructor to initialize your data members to some default state, then write a function called, say
bool Create(std::string filename);
And have create return true of false depending on if it succeeded or not.
or, do the work in the constructor, but check the object afterwards to see if it worked correctly.
TGATexture *tgaData = new TGATextureData("texture.tga"); // texture.tga does not exist!
if(!tgaData->succeeded) delete tgaData;// failed free the resources
Wisdom is knowing when to shut up, so try it.
--Game Development http://nolimitsdesigns.com: Reliable UDP library, Threading library, Math Library, UI Library. Take a look, its all free.
I think a better approach would be to throw a custom exception when the file is not valid and than catch that exception on the other side and handle it whatever way you want. That way your object will not be constructed and it is the proper way to handle scenario's like this.
Just my 2 cents, hope it helps.
Remember to mark someones post as helpful if you found it so.
TGATextureData* TryLoadTGATextureDataFromFile(const char *filename)
{
std::vector<char> data;
// loading the data from the file
if (!data.empty())
return new TGATextureData(data);
else
return 0;
}
Now your texture data class is more simpler. It is always in usable state, so you don't have to check IsLoaded() anymore. It is not tied to the source of the resource (the filesystem), and you could even drop the TGA part from it.
I think throwing an exception is the correct way to indicate a failure of construction.
Along with using RAII in order not to leak resources (fp in your case).
I think throwing an exception is the correct way to indicate a failure of construction.
Along with using RAII in order not to leak resources (fp in your case).
I think handling exceptions in C++ is more expensive than simply check a boolean value .... (?)
I think handling exceptions in C++ is more expensive than simply check a boolean value .... (?)
I'm not competent to debate that, but in this case, there is not even the need to debate: the OP didn't say that this code was to be as efficient as possible. The famous "premature optimization etc...".
And I doubt that texture loading code is called often.
