Jump to content
  • Advertisement
Sign in to follow this  
  • entries
    292
  • comments
    557
  • views
    154550

Exercise for the day:

Sign in to follow this  
TANSTAAFL

154 views

The following code is poorly written. How /SHOULD/ it be written? A big plate of virtual crawdads for the first person to get it right.


void ResourceManager::DeleteMap(ResMap &theMap)
{
for (ResMap::iterator anItr = theMap.begin(); anItr != theMap.end(); ++anItr)
delete anItr->second;

theMap.clear();
}



To help out: ResMap is a typedef for map, although this is readily apparent from the code.
Sign in to follow this  


18 Comments


Recommended Comments

void ResourceManager::DeleteMap(ResMap &theMap)
{
theMap.erase(theMap.begin(), theMap.end());
}

Share this comment


Link to comment
Quote:

void ResourceManager::DeleteMap(ResMap &theMap)
{
theMap.erase(theMap.begin(), theMap.end());
}


That won't call delete for the BaseRes*


void ResourceManager::DeleteMap(ResMap &theMap)
{
for (ResMap::iterator anItr = theMap.begin(); anItr != theMap.end(); ++anItr)
{
BaseRes* pBaseRes = static_cast<BaseRes*>( anItr->second );
if ( pBaseRes )
delete pBaseRes;

}

theMap.clear();
}

Share this comment


Link to comment
VisualLR is on the right track, but he's not quite there yet.

and noak, your solution would cause a memory leak.

Share this comment


Link to comment

void ResourceManager::DeleteMap(ResMap &theMap)
{
while(!theMap.empty()){
delete theMap.end()->second;
theMap.erase(theMap.end());
}
}


...I haven't tried it.

Share this comment


Link to comment
If it's allowed ..the resource could be a smart pointer. This would ensure automatic deletion of the resource using the code I posted above. Whaja thank?

Share this comment


Link to comment
And Rob has an access violation, because end() doesn't return an iterator with any data attached to it.

Thus far, the best I can come up with:


template <class value_t> void SafeDelete2nd(value_t& theValue)
{
if(theValue.second)
{
delete theValue.second;
theValue.second=0;
}
}

void ResourceManager::DeleteMap(ResMap &theMap)
{
for_each(theMap.begin(),theMap.end(),SafeDelete2nd<ResMap::value_type>);
theMap.clear();
}



Share this comment


Link to comment
Quote:
If it's allowed ..the resource could be a smart pointer. This would ensure automatic deletion of the resource using the code I posted above. Whaja thank?


Unfortunately, this is not a code design problem... this is a code maintenance problem. The code I originally posted actually exists, so going back and making the resources smart pointers would be a lot more work than simply checking for null when going through the list.

Share this comment


Link to comment
Do you have delete overloaded for type BaseRes?

Otherwise, there is no reason to verify that your pointer doesn't point to null, because that safety net is built into the standard C++ delete.

I guess it's an issue if you're using a compiler that doesn't follow that rule.

Share this comment


Link to comment
Actually, Jesse brings up a good point.

So, my "best" code for this gets modified:


template <class value_t> void SafeDelete2nd(value_t& theValue)
{
delete theValue.second;
theValue.second=0;
}

void ResourceManager::DeleteMap(ResMap &theMap)
{
for_each(theMap.begin(),theMap.end(),SafeDelete2nd<ResMap::value_type>);
theMap.clear();
}


I still do want something set to 0 after a deletion, but if it happens to be 0 already, go ahead, as delete doesn't do anything.

Mainly, my concern with the original code was the hand-rolled loop with the iterator instead of for_each.

Share this comment


Link to comment
Is that really going to gain you anything over this? I've never used for_each, and this is almost exactly what some of my code looks like, so I'm really curious.

ResMap::iterator anIter = theMap.begin();

while(anIter != theMap.end())
{
delete anIter->second;
++anIter;
}
theMap.clear();



While it's good form, you don't need to null-terminate the pointer in this case, since you make it irrelevant it by clearing the map anyway. (I've never used the source tags before, forgive me if this doesn't come through correctly.)

Share this comment


Link to comment
The reason I set the pointer to 0 in my template function is because I cannot guarantee that I will only be using that function when I'm about to clear the map.

The reason for using for_each is based on Item 43(pg 181) of Effective STL, by Scott Meyers, titled: "Prefer algorithm calls to hand-written loops."

He goes on for 9 pages of "why", which I won't repeat nor even summarize here, except to state that "he's right".

Share this comment


Link to comment
Actually, I'll quote his three reasons:

Quote:

Effective STL by Scott Meyers, Item 43, Page 182
Efficiency: Algorithms are often more efficient than the loops programmers produce.

Correctness: Writing loops is more subject to errors than is calling algorithms.

Maintainability: Algorithm calls often yield code that is clearer and more straightforward than the corresponding explicit loops.

Share this comment


Link to comment
More Effective C++ and Effective STL have been on my wish list for awhile. (I have Effective C++.) I think I'll finally get off my butt and order them.

And thanks for the page number, I'll look into it asap.

Share this comment


Link to comment
I have those books, I guess it's time for a re-read =)

I had forgotten about for_each, I've only used it when using functors, so it didn't immediately come to mind for this.

Share this comment


Link to comment
So.. do I get at least half a plate of virtual crawdads for almost getting it right?

Share this comment


Link to comment
Guest Anonymous Poster

Posted

I claim that setting it to null after deletion is useless! HA! ;)

Share this comment


Link to comment

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
  • Advertisement
×

Important Information

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

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!