Binary Tree Node Fault

Question

Here's the node definition:

struct node{
    int data;
    stuct node * left;
    struct node * right;
};

What I am trying to do is list all the nodes that point to an ancestor node. After posting the wrong solution and taking advice from the answers, my new solution is:

Recursively go through the binary tree. Add the current node to an array of nodes and then check if the children of the current node point to any of the previous ancestor nodes.

The default case is the node being NULL. If that happens the function returns.

How it is supposed to work:

Adds the node to the array

Checks if the left child is NULL.

If not, it compares the child to each of the previous nodes.

If it finds a fault, it reports it.

If not, it calls the function with the child as the argument.

Repeat until finished. (Does same for rhs of binary tree)

Questions:

• Is an array the best thing to store the nodes?
• Does this work? for (i = 0; i < sizeof(arrOfNodes) / sizeof(node); i++)
• Because the function is recursive, the array and the array index can't be initialized inside the function (or can they be?) so should they be global?
• Would it be better to have two arrays? (one for the LHS and one for the RHS)

The code:

void findFault(node * root){
    if (root == NULL){
      return;
    }

    arrOfNodes[index++] == root; // array of nodes

    if (root->left != NULL){
      for (i = 0; i < sizeof(arrOfNodes) / sizeof(node); i++){
         if (ar[i] == root->left){
             printf("%d", root->left);
             return;
         }
       }
       findFault(root->left);
    } else return;

    if (root->right != NULL){
      for (i = 0; i < sizeof(ar) / sizeof(node); i++){
         if (ar[i] == root->right){
             printf("%d", root->right);
             return;
         }
      }
      findFault(root->right);
    } else return;
}

Answer 1

I don't know about recursion, but this:

if (&root->left->left == &root){

is wrong in more ways that I can possibly describe, but anyway here are three issues:

• Why are you taking the address of root?
• Why don't you test that the first left pointer is null?
• You could simply use a std::map, but learning how to implement a binary tree is a good idea too.

Answer 2

This is an incorrect solution to the problem. Neil Butterworth already noted on your code, I'll note on the algorithm.

Your algorithm only checks a very specific case - whether a grandchild node points to its grandparent. What you should do is collect the parents along the way to a node and see that a node's child isn't one of its parents.

There are many ways to do this. One is to add a counter to your node struct and set all nodes' counters to zero before you begin traversing the tree. Whenever you reach a node you make sure the counter is zero and then increase it by one. This means that if you see a child whose counter isn't zero, you've already visited it and therefore the tree isn't valid.

Answer 3

I don't know if the algorithm that generates the binary tree is able to propagate a fault other than node's left/right child.

Anyway, this is a corrected version for your code:

void findFault(node * root){
    if (root == NULL){
      return;
    }

    if (root->left == root){
      printf("left: %d", root->data);
    } else findFault(root->left);

    if (root->right == root){
      printf("right: %d", root->data);
    } else findFault(root->right);
}

Answer 4

Another way to accomplish this kind of check is to do a breadth-first sweep of the nodes, all the while keeping a vector of nodes you have visited already (which you can keep sorted by address). Each time you visit a node, assert it is not in the vector, then add it to the appropriate place to keep the visited list sorted.

The advantage to this kind of check is it can be performed without modifying the tree or node struct itself, though there is a bit of a performance penalty.

Notes:

• An array would be a fine way to store the nodes. If you're avoiding STL (curious: why?) then you'll have to manage your own memory. Doable, but it's a brittle wheel to reinvent.
• Your for loop check to get the size of the arrays will not work; if you use malloc/free or new/delete then you'll have to specify the size of the array you want beforehand; you should use that size instead of calculating it every time through the for loop.
• The typical pattern for a recursive algorithm is to have an "outer" and "inner" function. The outer function is the one called by external code and does the initial setup, etc. The inner function is only called by the outser function, tends to have a more complicated parameter set (taking data set up by the outer function), and calls itself to perform the actual recursion.
• You will need two arrays: one for the list of nodes you have visited, and one for the list of nodes you have yet to visit.