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

## 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* 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 on other sites
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 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 on other sites
Quote:
 Original post by Evil Steve3. 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 on other sites
Quote:
Original post by swiftcoder
Quote:
 Original post by Evil Steve3. 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 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 on other sites
Quote:
 Original post by SiCraneFor 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 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 on other sites
Quote:
 Original post by SiCraneWhy 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 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.

1. 1
2. 2
Rutin
22
3. 3
4. 4
5. 5

• 16
• 14
• 9
• 9
• 9
• ### Forum Statistics

• Total Topics
632928
• Total Posts
3009264
• ### Who's Online (See full list)

There are no registered users currently online

×