Unable to print complete Binary Search Tree because my logic to iterate backwards from the lowest Node is flawed

Question

I have a program which is using for-loop to print all the elements in the BST in-order. I know that in order to do so, I need to print the left-Node, Parent Node, and then the right node. I can make such programs work when there is no for-loop involved. However, I am failing here to navigate backwards properly. In findNext() my logic is incorrect, because it prevents me from printing the complete tree. For example, if I have the following tree,

        101
     /        \
    51        161
   /  \      /  \ 
  31   91   141   171

Then my output would look something like the following (when I run the code which I pasted below).

31 
51 
101 
161 
171 

In the output above, I am missing the data for 91 and 141. I know why I am missing the data in these nodes ( because of findNext()) , but I can't figure out how to fix this issue using my logic. I am pasting the code below. I know I can probably use queues to solve this issue, but I think that's unnecessary because then I overcomplicating a simple problem.

include <stdio.h>
#include <stdlib.h>
#include <cstdio>


struct node {
    int value;
    node * left;
    node * right;
    node * parent;//Since we are given Parent node, we can keep track of root (previous) node.

    node( int value, node * left, node * right, node * parent=NULL ) :
        value( value ), left( left ), right( right ), parent( parent ) {}
};

// Find the minimum element in the BST. This function runs as expected.
// It only runs one time to find 31 (the lowest element in BST) 
struct node * findMin( struct node *n ){    
    if (n == NULL)
        return NULL;
    if (n->left == NULL)
        return n;
    return findMin(n->left);
}

//This function is supposed to provide the caller with the next available node to print.
//We are trying to print the elements in-order, so findNext() should first return 51, followed
//by 91, then followed by 101 etc. However, it is not returning 91. It returns 51, 101, 161 etc.
struct node * findNext( struct node *n ) {
    if (n == NULL)
        return NULL;
    if (n->parent)
    {
        // Logic: Return the parent node if we are in the left node. We know we are in the left node
        // because n->parent->left == n.
        if (n->parent->left == n)  
            return n->parent;

        // Logic: Return the right node if we are in the right node. This is wrong. I am lost after this point. 
        // I wanna be able to tell that I am in the Parent node or the right node. 
        // If I am in the parent node, then I wanna return the right node. If I am in 
        // right node, then I wanna return parent's parent. 
        if (n->parent->right == n)
            return n->right;
        else
            return n->left;
    }
    else
    {
        return n->right;
    } // I know how BST works, but I honestly can't figure out how to automate this without queues. 
}

// This function can be ignored, only part of setup process.
void connectparent( node * n, node * parent ) {
    if( n ) {
        n->parent = parent;
        connectparent( n->left, n );
        connectparent( n->right, n );
    }
}
// This function can be ignored, only part of setup process.
node * constructtree() {
    node * root = new node(101,
            new node( 51, new node( 31, NULL, NULL ), new node( 91, NULL, NULL ) ),
            new node( 161, new node( 141, NULL, NULL ),new node( 171, NULL, NULL ) ) );
    connectparent( root, NULL );
    return root;
}

int main() {
    node * root = constructtree();
    for( node * n = findMin( root ); n; n = findNext( n ) ) {
        printf( "%d \n", n->value );
    }
    return 0;
}

Answer 1

Although you already have an answer, I hope this information is useful. Iterating from anywhere in the tree can also be achieved with cookies. Would you consider using objects?

For the Node class, I have added a destroy function and the ability to link children automatically

#include <iostream>
#include <list>
#include <algorithm>

struct Node
{
  Node *parent;
  Node *left;
  Node *right;
  int value;

  Node(int value, Node *left=nullptr, Node *right=nullptr) : parent(nullptr), left(left), right(right), value(value)
  {
    if (left) { left->parent = this; }
    if (right) { right->parent = this; }
  }

  Node(const Node&) = delete;

 ~Node(void)
  {
    if (parent && (parent->left == this)) { parent->left = nullptr; }
    if (parent && (parent->right == this)) { parent->right = nullptr; }
    parent = left = right = nullptr;
    //...
    cout << "destroying node<" << value << ">...\n";
  }

  void destroy(void)
  {
    for (auto *ptr : {left, right})
    {
      if (ptr)
      {
        if (ptr->left || ptr->right) { ptr->destroy(); }
        delete ptr;
      }
    }
  }
};

I added a simple NodeTree class to manage the Nodes.

struct NodeTree
{
  Node *root;

  NodeTree(Node *root=nullptr) : root(root)
  {
    //
  }

 ~NodeTree(void)
  {
    clear();
  }

  void clear(void)
  {
    if (root) { root->destroy(); delete root; root = nullptr; }
  }

  Node* bottomLeft(void)
  {
    if (root == nullptr) return nullptr;

    for (auto *ptr = root; true; ptr = ptr->left)
    {
      if (ptr->left == nullptr) { return ptr; }
    }
  }
};

Finally, I added a NodeIterator class. This object would allow you move from any Node in any direction.

struct NodeIterator
{
  std::list<Node*> cookie; // keeps a list of visited nodes

  NodeIterator(void) {}
  NodeIterator(const NodeIterator&) = delete;
 ~NodeIterator(void) {}

  Node* findNext(Node *ptr, int flag = 0x3)
  {
    if (ptr)
    {
      // flag:
      // 0x1 -> search upstream for unvisited nodes
      // 0x2 -> search downstream for unvisited nodes

      if ((flag <= 0) || (flag > 0x3)) flag = 0x3;

      cookie.push_back(ptr);

      for (; (ptr != nullptr); ptr = ptr->parent)
      {
        if ((flag & 0x2) && ptr->left && (std::find(cookie.begin(), cookie.end(), ptr->left) == cookie.end())) return ptr->left; // move down
        if ((flag & 0x2) && ptr->right && (std::find(cookie.begin(), cookie.end(), ptr->right) == cookie.end())) return ptr->right; // move down
        if ((flag & 0x1) && ptr->parent && (std::find(cookie.begin(), cookie.end(), ptr->parent) == cookie.end())) return ptr->parent; // move up
        if ((flag & 0x1) == 0) break; // stop, if not allowed to search upstream
      }
    }

    return nullptr;
  }
};

For the main function

int main(int argc, char** argv)
{
  NodeTree N(new Node(101, new Node(51, new Node(31), new Node(91)), new Node(161, new Node(141), new Node(171))));

  NodeIterator iter;

  for (auto *ptr = N.bottomLeft(); (ptr != nullptr); ptr = iter.findNext(ptr))
  {
    cout << ">> " << ptr->value << '\n';
  }

  cout << "...............\n";

  return 0;
}

The output is:

>> 31
>> 51
>> 91
>> 101
>> 161
>> 141
>> 171
...............
destroying node<31>...
destroying node<91>...
destroying node<51>...
destroying node<141>...
destroying node<171>...
destroying node<161>...
destroying node<101>...

Answer 2

The fundamental bad assumption here is that findNext should take only one step (through one pointer). If you consider the nodes immediately before and after the root of a deep tree (note that these are not the root’s children!), obviously many parent links must be followed to reach the root, and then one right and many left nodes must be followed to step away from it. (In fact, that particular case is findMin(root->right), which suggests that they can be combined somehow.) With that in mind, and the recognition that the stepping up should stop when it was a step to the right (how would you tell?), it should be straightforward to design a correct BST stepping function.

Answer 3

I managed to find a solution which doesn't require me to change my signature of findNext().

findNext() would be what's shown below. If i use the code below, I am getting a complete BST with no nodes missing.

struct node * findNext( struct node *n ) { 
    if (n == NULL)
        return NULL;
    if(n->right) {
        return findMin(n->right);
    }   
    else {
        for(; n->parent; n=n->parent) {
            if(n->parent->left == n) {
                return n->parent;
            }   
        }   
        return NULL;
    }   
}