Binary Tree search returns no results (C++)

Question

I am working on some binary tree algorithms and need a "find node with searchindex..." function. The design for treenodes is basically

class TreeNode {
  int index; // some identifier
  TreeNode *left;
  TreeNode *right;
}

and a tree is defined by a pointer to the root-node.

My implementation for the search function is:

void Tree::searchNode(TreeNode * root, int nodeIndex, TreeNode *resultNode){
/* Recursive search */

  if (root->index == nodeIndex) {
            resultNode = root;
  } else {

    /*  search children if the current node is not a leaf */

    if(!root->isLeaf()) {
        this->searchNode(root->left,nodeIndex,resultNode);
        this->searchNode(root->right,nodeIndex,resultNode);
        }
  }
}

Arguments: *root is the root-node of the tree, nodeIndex is the search-index and *resultNode is the pointer to the found (or not) node in the tree.

The function does not return a reference or pointer to the found node but modifies the pointer resultNode so it points to the found node. The idea is to initialize resultNode with NULL, perform the search and modify it if a match occurs. Otherwise it remains NULL and I can easily check if there are search results or not.

Another class with a tree buildingTree as member utilizes the search-function in this way:

TreeNode *resultNodePtr = NULL;
this->buildingTree->searchNode(this->buildingTree->rootPtr, 
                               currentNodeIndex, resultNodePtr);

// do sth. with resultNodePtr if != NULL

I create *resultNodePtr on the stack because I just need it temporarily inside the function. Is this done correctly? However: The function does not work. resultNodePtr is always NULL, even if the tree contains a node with the search-index. I debugged it very carefully step by step, it detects

(root->index == nodeIndex)

correctly but

  resultNode = root;

does not work (I want resultNode to point to the same adress root points to). Debugger says resultNode before assignment is 0x0, root node is some adress, after the assignment resultNode remains 0x0.

Do I have to overload the operator= in this case for the class TreeNode?

I have tried it:

TreeNode & TreeNode::operator=(const TreeNode & oldTreeNode){
 *this = oldTreeNode;
 return *this;
 // ignore childs for now
}

I am not an expert but this operator= seems trivial. Does it affect the assignment of two TreeNode pointers *node1 = *node2 at all?

Maybe you can help me. Thanks for reading, appreciate your help. If I find a solution myself I will post it here.

Regards, Mark

Is there a reason to pass in the pointer and modify it? It seems cleaner just to use a return value, and it would avoid the problem explained in the answers below. — mwd, Aug 26 '11 at 17:49
Completely unrelated to your issue, but usually a tree is sorted, and the search can be implemented in a MUCH faster manner. Is there a reason that you search every node of the tree? As it is, your class searches slower than std::find on a std::list! — Mooing Duck, Aug 26 '11 at 19:03
Also, that operator= that you implemented is an infinite recursive loop. It should be something like `index = oldTreeNode.index; left=(oldTreeNode.left?new TreeNode(*oldTreeNode.left):nullptr); right=(oldTreeNode.right?new TreeNode(*oldTreeNode.right):nullptr);` — Mooing Duck, Aug 26 '11 at 19:06
And any class that has pointers that own heap allocated data should obey the rule of five: http://stackoverflow.com/questions/4782757/rule-of-three-becomes-rule-of-five-with-c0x. — Mooing Duck, Aug 26 '11 at 19:07
@mdw: Yes, a return value is better and now that I know the mistake I made I changed it back. — , Aug 26 '11 at 19:15
@Mooing Duck: Thanks for your ideas and links. You can speed up my function a bit if you bracket the code in "if (resultNode == NULL)" so it cancels immediately if a node is found. — , Aug 26 '11 at 19:20
@Mark is your tree sorted? Are the nodes in Left less/more than the nodes in right? If so, it's possible to search log(N) instead of (N/2) nodes, for a huge speed boost. — Mooing Duck, Aug 26 '11 at 19:22
`if(!root->isLeaf()) searchNode( (nodeIndexindex?root->left:root->right),nodeIndex,resultNode);` — Mooing Duck, Aug 26 '11 at 19:23
If your tree contains 1000 nodes, the above code will check ~32. — Mooing Duck, Aug 26 '11 at 19:25
@Mooing Duck: No, the distribution (left/right) is random. EDIT: Will check out your suggestion. :) — , Aug 26 '11 at 19:37
@Mark: Balanced trees are really hard. If you're not already doing one, don't switch. — Mooing Duck, Aug 26 '11 at 20:26

score 2 · Accepted Answer · answered Aug 26 '11 at 17:44

Because you pass resultNode into the function as a pointer by value, its original value never changes. Think of TreeNode* as literally nothing more than a number representing a memory address; when you reassign it:

resultNode = root;

This modifies the copy that searchNode has, but not the original pointer in the code which invokes searchNode. Take this simpler example:

void Foo(int x)
{
    x = 100;
}

void Bar()
{
    int x = 0;
    Foo(x);
    // at this point, x is still 0
}

resultNode's value doesn't change from NULL for the same reason that x doesn't change from 0 when the function Bar is invoked. To fix this issue, pass the pointer in as a pointer to a pointer, or a pointer by reference:

void Tree::searchNode(TreeNode* root, int nodeIndex, TreeNode*& resultNode)
{
    // same code
}

... or:

void Tree::searchNode(TreeNode* root, int nodeIndex, TreeNode** resultNodePtr)
{
    // assign to *resultNodePtr instead
}

Problem fixed. Thanks a lot for the detailed help. :) – Aug 26 '11 at 17:58 — , Aug 26 '11 at 17:58

score 0 · Answer 2 · answered Aug 26 '11 at 17:47

0

Your resultNode pointer is being passed by value, not by reference. So when the function call completes the pointer on the calling side does not receive a value.

Your algorithm looks fine :)

answered Aug 26 '11 at 17:47

lettucemode

422
4
14

Problem fixed. Thanks for your help! :) – Aug 26 '11 at 17:58

Binary Tree search returns no results (C++)

2 Answers2