How to stop tree search after child was found

Question

The following is failing to return the correct child node, even though it is actually finding the child further up the tree. It appears to drop the child after finding it, ad proceed to just keep searching the rest of the tree.

private Node<K, V> getNode(K key, ArrayList<Node<K, V>> children){
    if (children == null) return null;

    if (root.getKey().equals(key)) return root;

    for (Node<K, V> child : children) {
        if (child.getKey().equals(key)) return child;
        getNode(key, child.getChildren());
    }

    return null;
}

I tested it with the following code:

Tree<Integer, String> tree = new Tree<>(1, "1");

tree.addChild(1, new Node<>(2, "2"));
tree.addChild(1, new Node<>(3, "3"));
tree.addChild(1, new Node<>(4, "4"));
tree.addChild(2, new Node<>(5, "5"));

System.out.println(tree.addChild(5, new Node<>(6, "6")));
System.out.println(tree.addChild(5, new Node<>(7, "7")));

However, the console outputs false both times, even though it should be true. The child with a key of 5 cannot be found, even though I added one to the tree.

Answer 1

The problem is that when you look for a child in a subtree, you ignore the return value:

if (child.getKey().equals(key))
{
    // This is fine
    return child;
}
else
{
    // This is bad: you ignore the return value.
    getNode(key, child.getChildren());
}

To fix, capture the return value, and return it if it is not null, like this:

if (child.getKey().equals(key))
{
    return child;
}
else
{
    Node<K, V> res = getNode(key, child.getChildren());
    if (res != null) {
        return res;
    }
}

In addition, your code will miss the situation when the value is stored at the root, and the root has no children, because the root.getKey().equals(key) is not done on nodes that have no children.

Answer 2

write return getNode(key, child.getChildren()) in else statement. Its the way to work with recursion.

      ....
      else
      {
       return getNode(key, child.getChildren());
      }

Answer 3

After major reformatting work, I have refactored your code into a much more legible form. While substantially different, the following is logically identical to your code:

private Node<K, V> getNode(K key, ArrayList<Node<K, V>> children){
    if (children == null) return null;

    if (root.getKey().equals(key)) return root;

    for (Node<K,V> child : children) {
        if (child.getKey().equals(key)) return child;
        getNode(key, child.getChildren());
    }

    return null;
}

Now, we can pick apart the code and fix it.

The first problem is that you don't have a javadoc comment in front of the method documenting its parameters and return value with @param and @return. That is someting you need to fix.

Second off, this method should be implemented as a class method of the Node class, and should be public. That is:

class Node<K,V> {

// ... whatever else this class has in it ... 

    public K getKey() { /* ... stuff ... */ }

    ArrayList<Node<K,V>> children = new ArrayList<>();

    public Node<K, V> getNode(K key){
        if (children == null) return null;

        if (key.equals(this.getKey())) return this;

        for (Node<K,V> child : children) {
            if (child.getKey().equals(key)) return child;
            child.getNode(key);
        }

        return null;
    }
}

Also, since we have now guaranteed that children always gets initialized, and is entirely under our control, we can get rid of the bogus null check.

public Node<K, V> getNode(K key){    
    if (key.equals(this.getKey())) return this;

    for (Node<K,V> child : children) {
        if (child.getKey().equals(key)) return child;
        child.getNode(key);
    }

    return null;
}

Now, you are redundantly checking the children. Since getNode() already checks if this is the correct node, there is no reason to separately check each child of the current node:

public Node<K, V> getNode(K key){    
    if (key.equals(this.getKey())) return this;

    for (Node<K,V> child : children)
        child.getNode(key);

    return null;
}

Now that we have gotten rid of this much code, it should be fairly obvious what the problem actually is: the upper method never actually does anything with the node turned up by searching the child. A simple change is enough to fix this though:

public Node<K, V> getNode(K key){    
    if (key.equals(this.getKey())) return this;

    for (Node<K,V> child : children){
        Node<K,V> result = child.getNode(key);
        if(result != null) return result;
    }

    return null;
}

Note that we should not be checking if the child is null. This should be handled by the method we expose for adding new values, and we should never be adding foreign nodes to our tree:

public boolean put(K key, V value){
    children.add(new Node<>(key, value));
}

And one more thing: There is no need for a seperate Tree class at all! You should not have one, all its functionality should exist entirely within the node class. Ideally, the root node is the tree.