Refactoring code finding if Binary Tree is BST I have written an code to find if a given tree is a BST. Need help in refactoring it. Some of the things I am looking are.


Getting Rid of temp variables (As suggested by Martin Fowler)
Combining leftTreeMax and RighTreeMin methods
Better naming of methods.


Also it will be great if someone can suggest other refactorings which i can do. 

package com.cc.bst;

import com.cc.queue.Queue;
import com.cc.trees.BinaryTreeNode;

public class IsBinaryTreeBST {


public static boolean binaryTreeBST ( BinaryTreeNode root) {

    if (root == null) {
        return false;
    }
    int maxVal = leftTreeMax(root.getLeft());
    int minVal = rightTreeMin(root.getRight());
    int rootVal = root.getData();

    if (maxVal == 0 || minVal == 0 ) {
        return false;
    }

    if (rootVal &gt; maxVal &amp;&amp; rootVal &lt; minVal) {
        return true;
    }

    return false;

}


private static int leftTreeMax (BinaryTreeNode node) {

    if (node == null) {
        return 0;
    }
    Queue nodeQueue = new Queue();
    nodeQueue.enqueue(node);
    int maxValue = node.getData();

    while (!nodeQueue.isEmpty()) {
        BinaryTreeNode tempNode = (BinaryTreeNode) nodeQueue.dequeue();         
        BinaryTreeNode left = tempNode.getLeft();
        BinaryTreeNode right = tempNode.getRight();

        if (left != null ) {
            if (left.getData() &gt; tempNode.getData()) {
                return 0;
            }
            nodeQueue.enqueue(left);
        }
        if (right != null) {
            if ( tempNode.getData() &gt; right.getData()) {
                return 0;
            }
            nodeQueue.enqueue(right);
        }
        if (tempNode.getData() &gt; maxValue) {
            maxValue = tempNode.getData();
        }           
    }       
    System.out.println("---------- maxVal --------&gt;" +  maxValue);
    return maxValue;
}

private static int rightTreeMin(BinaryTreeNode node) {

    if (node == null) {
        return 0;
    }
    Queue nodeQueue = new Queue();
    nodeQueue.enqueue(node);
    int minValue = node.getData();

    while (!nodeQueue.isEmpty()) {
        BinaryTreeNode tempNode = (BinaryTreeNode) nodeQueue.dequeue();         
        BinaryTreeNode left = tempNode.getLeft();
        BinaryTreeNode right = tempNode.getRight();
        System.out.println("---------- tempNode --------&gt;" + tempNode.getData());

        if (left != null ) {
            System.out.println("---------- left --------&gt;" + left.getData());

            if (left.getData() &gt; tempNode.getData()) {
                return 0;
            }
            nodeQueue.enqueue(left);                
        }
        if (right != null) {
            if ( tempNode.getData() &gt; right.getData()) {
                return 0;
            }
            System.out.println("---------- right --------&gt;" + right.getData());

            nodeQueue.enqueue(right);               
        }           
        if (tempNode.getData() &lt; minValue) {
            minValue = tempNode.getData();
        }
    }       
    System.out.println("---------- minVal --------&gt;" +  minValue);


    return minValue;
}   

}
1) You would like to get rid of the System.out.println() inside methods. Noone expects a method to write to the console unless the method has print in the name.

2) Just comparing the root with left tree max and right tree min can lead to wrong result. Consider,

    12
   /  \
  9    15
 / \
3   5


Here the left sub-tree is not a BST but 12 &gt; 9 and 15 &gt; 12. You must also ensure that left-subtree and right-subtree of root are also BST.

3) BinaryTreeBST is kind of redundant. Consider naming the class IsBSTCheck or IsBinarySearchTreeCheck. Also change the method name binaryTreeBST to isBST or isBinarySearchTree to reflect what the method does.