patternjavaMinor
Refactoring code finding if Binary Tree is BST
Viewed 0 times
refactoringbstbinaryfindingcodetree
Problem
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.
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 > maxVal && rootVal tempNode.getData()) {
return 0;
}
nodeQueue.enqueue(left);
}
if (right != null) {
if ( tempNode.getData() > right.getData()) {
return 0;
}
nodeQueue.enqueue(right);
}
if (tempNode.getData() > maxValue) {
maxValue = tempNode.getData();
}
}
System.out.println("---------- maxVal -------->" + 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 -------->" + tempNode.getData());
if (left != null ) {
System.out.println("---------- left -------->" + left.getData());
if (left.getData() > tempNode.getData()) {
re
- 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 > maxVal && rootVal tempNode.getData()) {
return 0;
}
nodeQueue.enqueue(left);
}
if (right != null) {
if ( tempNode.getData() > right.getData()) {
return 0;
}
nodeQueue.enqueue(right);
}
if (tempNode.getData() > maxValue) {
maxValue = tempNode.getData();
}
}
System.out.println("---------- maxVal -------->" + 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 -------->" + tempNode.getData());
if (left != null ) {
System.out.println("---------- left -------->" + left.getData());
if (left.getData() > tempNode.getData()) {
re
Solution
1) You would like to get rid of the
2) Just comparing the
Here the left sub-tree is not a BST but
3)
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 5Here the left sub-tree is not a BST but
12 > 9 and 15 > 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.Code Snippets
12
/ \
9 15
/ \
3 5Context
StackExchange Code Review Q#15785, answer score: 2
Revisions (0)
No revisions yet.