Counting number of nodes in Binary Search Tree C++

Question

I am struggling to understand why the function CountNodes() below counts all the nodes in a BST.

If we assume we have the following BST:

            20
          /   \
         10    30
        /  \  /  \
       5  15 25  35

If I call CountNodes(pointer to root node 20); then wouldn't the relevant if statement:

    if(root->left!=NULL)
    {
        n=n+1;
        n=CountNodes(root->left);
    }

simply look at node 10 and say, yes it is not null, add 1 to the counter n, and then call CountNodes(pointer to 10) which would then just send us down the left branch again to 5. Then when at 5 the left and right variables are NULL and hence the whole CountNodes function just returns n equal to int 3.

I guess I am struggling to understand exactly when the value of the argument to CountNodes is updated. Do we look right and check if its NULL and update the counter before we update the argument value in the first recursive call to CountNodes(pointer to 10) in the left look even though the right look appears after the left recursive call in the code?

#include<iostream>
using namespace std;

int n=1;

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

struct node* getNode(int data)
{
    node* newNode=new node();
    newNode->data=data;
    newNode->left=NULL;
    newNode->right=NULL;
    return newNode;
}

struct node* Insert(struct node* root, int data)
{
    if (root == NULL)
        return getNode(data);

    if (data < root->data)
        root->left  = Insert(root->left, data);

    else if (data > root->data)
        root->right = Insert(root->right, data);

    return root;
}


int CountNodes(node*root)
{
    if(root==NULL)
        return 0;

    if(root->left!=NULL)
    {
        n=n+1;
        n=CountNodes(root->left);
    }

    if(root->right!=NULL)
    {
        n=n+1;
        n=CountNodes(root->right);
    }
    return n;
}

int main()
{  
    node* root=NULL;
    root=Insert(root,10);
    Insert(root,5);
    Insert(root,20);
    Insert(root,4);
    Insert(root,8);
    Insert(root,15);
    Insert(root,25);
    cout<<"Total No. of Nodes in the BST = "<<CountNodes(root)<<endl;

    return 0;
}

I would avoid the global variable `n`. It's quite wrong using global variables with side effects in the recursion... — BiagioF, Nov 27 '19 at 17:41
I suggest you try to find a solution where `n` is a *local* variable instead of global. — Some programmer dude, Nov 27 '19 at 17:41
At first, you *absolutely* should define n as a local variable of `countNodes`. Then by *assigning* n as result of recursive call, you lose previous information. You'd need `+=` instead... — Aconcagua, Nov 27 '19 at 17:41
Also by doing `n = ...` followed by `n = ...` you lose the first assignment. — Some programmer dude, Nov 27 '19 at 17:42
Actually, you can do it really simple: `return root == nullptr ? 0 : 1 + count(root->left) + count(root->right);` – the null-check in next recursive call compensates the dropped check for children being available. `1 + ...` counts current node itself, which replaces the `n = n + 1` you have now. — Aconcagua, Nov 27 '19 at 17:49
`struct node*` is pretty much C. If you are writing c++, omit the `struct` keyword. (i.e. use `node*` instead) — ph3rin, Nov 27 '19 at 17:50
Are you aware that `std::set` (or `std::map`) actually is a binary search tree as well? Typically implemented as red-black-tree, so you won't get degenerate trees either... (degenerate tree: what would your implementation produce on inserting *sorted* data???). — Aconcagua, Nov 27 '19 at 17:52
Off-topic: You should prefer C++ *keywords* (`nullptr`) over old (obsolete?) C *macros* (`NULL`). — Aconcagua, Nov 27 '19 at 17:57
You should replace `getNode` function with a constructor: `node(int data) : data(data), left(nullptr), right(nullptr) { }` – then you'd create new nodes by simply `return new node(data);` — Aconcagua, Nov 27 '19 at 17:59

Martin York · Answer 1 · 2019-11-27T18:05:01.140

You are overwritting the value of n

    n=CountNodes(root->left);

You should be adding the count from the sub tree.

    n = n + CountNodes(root->left);

There is also another bug in that you are counting this node twice if the node has a left and right tree and never counting it if the node has no subtrees.

if(root->left!=NULL)
{
    n=n+1;                      // Adding one for this node.
    n=CountNodes(root->left);
}

if(root->right!=NULL)
{
    n=n+1;                      // Adding one for this node again?
    n=CountNodes(root->right);
}

I think you should be doing:

if(root->left!=NULL)
{
    n = n + CountNodes(root->left);
}

if(root->right!=NULL)
{
    n = n + CountNodes(root->right);
}
n = n + 1;

The next thing is that you check for null on entry into the function. So you don't need to test for null before doing a recursive call.

n = n + CountNodes(root->left);
n = n + CountNodes(root->right);
n = n + 1;

We can then simplify this to:

int CountNodes(node* root)
{
    if (root == nullptr) {
        return 0;
    }
    return 1 + CountNodes(root->left) + CountNodes(root->right);
}

Actually, you don't even need those two if's, if you drop them, the null-check for root in recursive call will catch up and thus resulting in n += 0... — Aconcagua, Nov 27 '19 at 18:02

Counting number of nodes in Binary Search Tree C++

1 Answers1