Back to General and Gameplay Programming

c++ : Peer review if anyone would be so kind and/or is bored:)

Guthur · 2008-11-26T16:01:23

Just have this piece of recursive code and i'd like it reviewed by another set of eyes :) Seems to work but I'm scared i have missed something. eg. is it ok to pass a queue like i did. I just want to make sure i haven't overlooked something major :s FindSubNode is called to find a Sub node of root in a xml tree. Thanks, XMLNode* ColladaLoader::FindSubNode( wstring nodeName, XMLNode* rootNode ) { XMLNode* xn = 0; queue<XMLNode*> parents; xn = SearchBreadth( nodeName, rootNode->GetFirstChild(), &parents ); while(!xn && parents.size() > 0) { XMLNode* tn = (XMLNode*)parents.front(); parents.pop(); xn = FindSubNode( nodeName, tn ); } return xn; } XMLNode* ColladaLoader::SearchBreadth( wstring nodeName, XMLNode* node, queue<XMLNode*>* parents ) { XMLNode* xn = 0; if( node ) { if( node->m_strName == nodeName ) { xn = node; //found our node } else { if( node->GetFirstChild() ) parents->push( node ); //is a parent xn = SearchBreadth( nodeName, node->GetNextNode(), parents); } } return xn; }

General and Gameplay Programming Programming

Started by Guthur November 26, 2008 02:12 PM

12 comments, last by Guthur 15 years, 5 months ago

Antheus

2,410

November 26, 2008 03:07 PM

Why is FindSubNode member of loader, and not node? It has nothing to do with Collada, but everything with XML node. It would make implementation somewhat simpler.

Guthur

101

Author

November 26, 2008 03:17 PM

Quote:Original post by Antheus
Why is FindSubNode member of loader, and not node? It has nothing to do with Collada, but everything with XML node. It would make implementation somewhat simpler.

I was just thinking that myself when i was adding it :), not a big deal i can move it, this was actually more a means to something else but i wanted to have it check over by the good people here :p

Also guys thanks for bringing up the Size() operation that info is good to have, :)

also the const string was good advice thanks. I really do need to make my code more strict.

Innovation not reiterationIf at any point I look as if I know what I'm doing don't worry it was probably an accident.

visitor

643

November 26, 2008 03:42 PM

Quote:
Hmm, fair point. It just seems to me that size() is a much more common function to see in code than splice(). Then again, maybe it's just the code I see

If you have chosen to use a std::list, then probably you are not interested in size(), but interested in the relinking capabilities (e.g splice) - e.g moving a whole list onto another one if the original is not needed any more.

In any case, if you mean to test if a container is empty (or not empty), empty() conveys the meaning better and is performance-wise faster than or equal to size().

Guthur

101

Author

November 26, 2008 04:01 PM

I have implemented the changes as suggested; using empty() now.

Thanks alot everyone, thorough as always :)

Edit: if anyone is interested i removed the allocation of 'tn' from the while loop, its just a reassignment now, not sure if it would make a big difference.

Innovation not reiterationIf at any point I look as if I know what I'm doing don't worry it was probably an accident.

c++ : Peer review if anyone would be so kind and/or is bored:)

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

c++ : Peer review if anyone would be so kind and/or is bored:)

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines