-1

I'm writing an iterative function to search a binary tree for a certain value. This is localized to signed ints until I get into how to genericize classes.

Assume that my class is BinarySearchTree, and it has a pointer to the root node of the tree. Also assume that nodes are inserted through an insert function, and have pointers to two children. Here is a much abbreviated version of the Node struct:

struct Node
{
   public:
      Node *left_, *right_;
      int value_

      Node(int val) : value_(val), left_(0), right_(0) { }
      //done in this manner to always make sure blank children are
      //init to zero, or null
      Node(int val, Node *left, Node *right) : value_(val), left_(0), right_(0) 
          { left_ = left; right_ = right; } 
}

So, you can safely assume that a node's uninit pointers will be NULL.

Here is my code:

int BinarySearchTree::search(int val)
{
    Node* next = this->root();

    while (next->left() != 0 || next->right () != 0)
    {
        if (val == next->value())
        {
            return next->value();
        }    
        else if (val < next->value())
        {
            next = next->left();   
        }
        else if (val > next->value())
        {
            next = next->right();
        }
    } 

    //not found
    return 0;
}

This code is being rejected by a friend for two reasons:

1) If next has no children, both will evaluate to zero and I will prematurely exit the loop (I will never check the searched val against next's value).

2) If next has one child, but the data you are searching for should be on the empty side of the tree, next will be set to 0, and it will loop again, comparing next (which is 0) to the left and right trees like while(0->left()), resulting in undefined behavior.

I am told that the solution to both problems lies in the loop condition, but I can't see what I can do to easily remedy the situation. Can the community of Stack Overflow offer any insights?

Tom
  • 21,468
  • 6
  • 39
  • 44
jkeys
  • 3,803
  • 11
  • 39
  • 63
  • I resent adding the homework tag. This is not homework, I am not in school, this is a hobby. – jkeys Jul 11 '09 at 03:51
  • @Hooked: I just added the algorithm tag. Also, I realize I just posted almost the same code, but read my post... consider returning bool instead of int. – Tom Jul 11 '09 at 04:01
  • @Hooked: No offense intended. I simply misunderstood the "code being rejected". Since your friend doing the rejecting didn't give you a reason why I assumed it was because it was homework -- where giving away the answer would be bad form. – tvanfosson Jul 11 '09 at 04:27
  • @Hooked: Do you understand why I suggest you change your return value to bool? What happens when you search for 0? How will you know if 0 was found, or if nothing was found? You will write code like "if (tree.Search(0) == 0)", but that's wrong. even though "if (tree.Search(1) == 1)" would work, it's awkward. You want to be able to write, "if (tree.Search(0))"... Does this make sense? – Tom Jul 11 '09 at 04:48
  • @tvanfosson: It's fine, but homework has the connotation that you aren't willing to do your own work. I am doing this as a hobby simply because it is quite fun. I am now into the 22nd page (20 posts/page) of a forum thread with two mutual friends (and programmers) teaching me how to program. – jkeys Jul 11 '09 at 04:49
  • Tom, I understand, I did change the code. I experienced a logic fail - I wrote the search function pretty much correctly, but did not think about the purpose of the function. I realize you want to eval to true if it exists, and false if it doesn't. – jkeys Jul 11 '09 at 04:50

3 Answers3

2

I think you should be testing if next is not NULL in your loop like so:

int BinarySearchTree::search(int val)
{
    Node* next = this->root();

    while (next)
    {
        if (val == next->value())
        {
            return next->value();
        }    
        else if (val < next->value())
        {
            next = next->left();   
        }
        else if (val > next->value())
        {
            next = next->right();
        }
    } 

    //not found
    return 0;
}
Tom Dalling
  • 23,305
  • 6
  • 62
  • 80
  • No, why would I want that? If it evaluates to null, then I should return for not having found the value earlier. – jkeys Jul 11 '09 at 03:47
  • It does return. The loop breaks and it returns 0; – Tom Dalling Jul 11 '09 at 03:48
  • In your example, while (next) would only evaluate to true if next is NULL, since NULL == 0 in C++. So your example is the complete opposite of what is needed, I think. – jkeys Jul 11 '09 at 03:49
  • No, it evaluates to true if next IS NOT null. NULL is false (i.e. 0). – Tom Dalling Jul 11 '09 at 03:50
  • Ha... I just wrote the same answer (almost)... I don't know how I did not see that you wrote this. One thing to the OP though... He should probably return bool, for the reason I mentioned in my post. – Tom Jul 11 '09 at 04:00
  • Errr, shouldn't you be returning the actual node OR a bool and not the value (int) you're looking for? – tomzx Jul 11 '09 at 04:28
  • @tomzx... I said the same thing... I pity the person using this function and searching for 0 :-P. – Tom Jul 11 '09 at 04:42
  • @tomzx... also, I wouldn't recommend returning the node, because you wouldn't want someone to have a handle to the internals of the tree. If anything, *maybe* a copy of the node would be good. But that seems like unnecessary construction. – Tom Jul 11 '09 at 04:49
  • @Tom hey Tom, learned a lot in the past decade. I'm now a full-stack engineer at a start-up thanks in part to answers like this. (I eventually did go to school for this and graduated w/ a BS in CS. :P ) – jkeys Oct 23 '19 at 00:40
  • @jkeys That's awesome! Congrats for sticking with it all these years, and I hope you're enjoying the industry. – Tom Dalling Oct 26 '19 at 13:34
1

Try this:

while (next != NULL) ?

Somnath Muluk
  • 55,015
  • 38
  • 216
  • 226
Mike Mu
  • 977
  • 6
  • 10
0

First of all, I'm not sure why you are returning an int. What if you are searching for 0 in the tree. You probably want something like this:

bool BinarySearchTree::Search(int val) {
  Node* current = root();
  while (current != NULL) {
    // Check if it's here
    if (val == current->value()) {
      return true;
    }
    if (val < current->value()) {
      current = current->left();
    } else {
      current = current->right();
    }
  }
  // Not found
  return false;
}

Notice that the loop invariant: at the beginning of each loop, you are at a non null node that you need to "process". First check if it's the node you want. If not, make a branch, and let the loop decide if the branch was "good" (ie - non null). Then you'll let the next loop iteration take care of testing.

Tom
  • 21,468
  • 6
  • 39
  • 44