Jump to content
  • Advertisement
Sign in to follow this  
Guthur

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

This topic is 3612 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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

Share this post


Link to post
Share on other sites
Advertisement
At a quick glance:
1. I'd pass the strings as const reference instead of value, to avoid making a copy for every function call.
2. You'd be as well passing the queue to SearchBreadth as a reference instead of a pointer.
3. Personally, I'd use !parents.empty() instead of parents.size() > 0 since it's a little easier to read.
4. Is this cast really nessecary: (XMLNode*)parents.front() ? If not, it should probably be removed.

Nothing majorly wrong that I can see though.

Share this post


Link to post
Share on other sites
- wstring should be passed by const reference
- assigning 0 to xn and then two lines later assigning the return from SearchBreath is a waste; declare and assign at the same time is possible.
- I would probably have SearchBreath() take the parents notes by reference
- why are you asking to XMLNode* from the parents.front() return?

Share this post


Link to post
Share on other sites
Quote:
Original post by Evil Steve
3. Personally, I'd use !parents.empty() instead of parents.size() > 0 since it's a little easier to read.
Also worth noting is that size() is not always a constant time operation for std::containers. In particular std::queue can be used with std::list as the underlying container, and common std::list implementations do not cache the length, instead requiring a linear time iteration to determine it.

Share this post


Link to post
Share on other sites
Quote:
Original post by swiftcoder
Quote:
Original post by Evil Steve
3. Personally, I'd use !parents.empty() instead of parents.size() > 0 since it's a little easier to read.
Also worth noting is that size() is not always a constant time operation for std::containers. In particular std::queue can be used with std::list as the underlying container, and common std::list implementations do not cache the length, instead requiring a linear time iteration to determine it.
I was going to mention that, but I wasn't sure if it was guaranteed to be O(1) or not. Does std::list really not cache the length? I thought any STL container not caching the length would be a fairly bad idea. I could understand it on VC6 though [smile]

Share this post


Link to post
Share on other sites
For std::list, the implementer has two choices: he can make size() O(1) or he can make splice() O(1). You can't do both.

Share this post


Link to post
Share on other sites
Quote:
Original post by SiCrane
For std::list, the implementer has two choices: he can make size() O(1) or he can make splice() O(1). You can't do both.
One of the implementations of splice() can, but surely size() is likely to be called more frequently than splice()? I wouldn't be surprised to see a call to size() in a for() loop's condition in some professional code even.

Share this post


Link to post
Share on other sites
Why would size() be called more frequently than splice()? You can iterate using begin() and end() and test for emptiness with empty().

Share this post


Link to post
Share on other sites
Quote:
Original post by SiCrane
Why would size() be called more frequently than splice()? You can iterate using begin() and end() and test for emptiness with empty().
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 [smile]

Share this post


Link to post
Share on other sites
Don't forget that having an O(1) size() means updating a cached size value in more places than just splice(). insert(), erase() and other modifiers will also need to do extra work to update the value, and you increase the size of the class when you keep that size number around.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

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

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!