• entries
292
557
• views
154550

# Exercise for the day:

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.

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

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();
}



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

and noak, your solution would cause a memory leak.

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


...I haven't tried it.

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?

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();
}



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.

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.

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.

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.)

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".

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.

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.

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.

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

Everybody who played can have as many virtual crawdads as they can eat.

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

## Create an account

Register a new account