Generic Binary Search Tree class written in C#

Question

using System;
using System.Collections.Generic;
using System.Linq;

namespace project
{
    class Program
    {
        public static void Main(string[] args)
        {
            BinarySearchTree<int> bst = new BinarySearchTree<int>();
            bst.Insert(1);
            bst.Insert(3);
            bst.Insert(5);
            bst.Insert(2); 
            bst.Insert(21);
            bst.Insert(4, bst.root); 
            System.Console.WriteLine(bst.FindByValue(4,bst.root).data);
        }
    }

    public class BinNode<T> where T:IComparable
    {
        //constructors
        public BinNode(T data)
        {
            this.data = data;
        }
        public BinNode() { }
        //properties 
        public BinNode<T> right {get; set;} = null;
        public BinNode<T> left {get; set;} = null;
        public T data {get;set;}
    }


    public class BinarySearchTree<T> where T:IComparable
    {
        public BinNode<T> root {get; set;} = null; 

        //recursively insert new node into the bst instance with T data 
        public void Insert(T data, BinNode<T> node)
        { 

            if (data.CompareTo(node.data) > 0)
            {
                if (node.right == null)
                {
                    node.right = new BinNode<T>(data); 
                    return;
                }
                Insert(data, node.right);
                System.Console.WriteLine("right");
            }

            else if (data.CompareTo(node.data) < 0)
            {
                if (node.left == null)
                {
                    node.left = new BinNode<T>(data);
                    return;
                }
                Insert(data, node.left);
                System.Console.WriteLine("left");
            }
        }

        //normal insertion method (inserts T data into a bst instance)
        public void Insert(T data)
        {
            if (root == null)
            {
                root = new BinNode<T>(data);
            }

            BinNode<T> current = root; 

            while (current != null)
            {
                if (data.CompareTo(current.data) > 0)
                {
                    if (current.right != null)
                    {
                        current = current.right; 
                        continue;
                    }
                    current.right = new BinNode<T>(data); 
                } 

                else if (data.CompareTo(current.data) < 0)
                {
                    if (current.left != null)
                    {
                        current = current.left;
                        continue;
                    }
                    current.left = new BinNode<T>(data); 
                }

                else
                {
                    return;
                }

            }
        }

        //insert all elements from an array
        public void InsertFromArray(T[] array)
        {
            foreach(T t in array)
            {
                this.Insert(t); 
            }
        }
        //get the last level in the bst instance
        public int GetLevel(BinNode<T> node, int current=1)
        {
            int right = 0 ;
            int left = 0 ;            

            if (node.right != null)
            {
                right =  GetLevel(node.right, current+1);
            }

            if (node.left != null)
            {
                left = GetLevel(node.right, current+1); 
            }

            if (right == 0 && left == 0) return current; //this is readable in my opinion but according to best practices (microsoft), only one operation per line is good, so should this be changed or is it okay

            else 
            { 
                return right > left ? right : left; //is this acceptable?
            }

        }
        //get a queue of all the values in a given level
        public void GetOnLevel(BinNode<T> node, int curLevel, int trgLevel, Queue<T> result)            
        {
            if (curLevel == trgLevel)
            {
                result.Enqueue(node.data);
            }
            else
            {
                if (node.left != null)
                {
                    GetOnLevel(node.left, curLevel+1, trgLevel, result);
                }

                if(node.right != null)
                {
                     GetOnLevel(node.right, curLevel+1, trgLevel, result); 
                }
            } 
        }

        //returns the node that holds data equivalent to T data
        public BinNode<T> FindByValue(T data, BinNode<T> node)
        {
            if (node == null) return null; 

            if (data.Equals(node.data))
            {
                return node;
            }

            if (data.CompareTo(node.data) > 0)
            {
                return FindByValue(data, node.right);
            }
            else if (data.CompareTo(node.data) < 0)
            {
                return FindByValue(data, node.left); 
            }

            else return null; 
        }

    }
}

I'm wondering whether the lines

if (right == 0 && left == 0) return current;

… and

return right > left ? right : left;

are acceptable for readability and maintainability. As well as any other improvements I can make to my code.

Answer 1

I'm wondering whether the lines

if (right == 0 && left == 0) return current;

… and

return right > left ? right : left;

are acceptable for readability and maintainability.

In my option they aren't but not because of the conditions but because of the suboptimal naming of the variables. I had to look twice at your code to understand what you are doing here as you usually use left and right to adress the nodes. Here however you are talking levels:

int right = 0 ;
int left = 0 ;            

if (node.right != null)
{
    right =  GetLevel(node.right, current+1);
}

if (node.left != null)
{
    left = GetLevel(node.right, current+1); 
}

Name them rightLevel and leftLevel and I think it should be clear what their purpose is.

---

Some more observations about the rest of the code:

Be consistent, if you call the main class BinarySearchTree then don't name the other one just BinNode but BinaryTreeNode

---

public BinNode<T> right { get; set; } = null;
public BinNode<T> left { get; set; } = null;

It's not necessary to set anything to null, it's null by default.

---

public BinNode(T data)
{
    this.data = data;
}

public T data { get; set; }

It's pointless to have both the constructor and a get/set property. Make the property either readonly and required via constructor or leave it optional and get rid of the construtor.

---

All public members should be named with PascalCase unlike left/right/data where the convention is usually that data is to be called Value.

---

Don't put everything in a single class. Extract the FindByValue method into a new one that you could call BinaryTreeSearch and rename the old one to just BinaryTree.

---

You can make your life much easier if you override a few operators like <, >, == and != so that you don't have to use CompareTo everywhere.

---

node.left = new BinNode<T>(data);

Use helper variables so you don't have to repeat yourself so much.

---

In

public void InsertFromArray(T[] array)

and then

foreach (T t in array)

you shouldn't use abbreviations like t or general names like array. Call it item and source. In fact this could be even an IEnumerable and be an extension. A general convention is to call it AddRange or in this case InsertRange.

---

public void GetOnLevel(BinaryTreeNode<T> node, int curLevel, int trgLevel, Queue<T> result)

More ugly abbreviations.

---

Example

So, this is how it could look when optimized a little bit. I'm not saying it's pefect yet but I find it's already much better and easier to read then before.

This is the new BinaryTreeNode class that now implement all suggestions:

public class BinaryTreeNode<T> where T : IComparable
{
    public BinaryTreeNode<T> Right { get; set; }
    public BinaryTreeNode<T> Left { get; set; }
    public T Value { get; set; }

    public override bool Equals(object obj)
    {
        if (obj is BinaryTreeNode<T> other)
        {
            if (ReferenceEquals(Value, other.Value)) return true;
            return Value.Equals(other.Value);
        }
        return false;
    }

    public override int GetHashCode() => Value == null ? 0 : Value.GetHashCode();

    public static bool operator ==(BinaryTreeNode<T> left, BinaryTreeNode<T> right) => ReferenceEquals(left, right) || (left?.Equals(right) ?? false);
    public static bool operator !=(BinaryTreeNode<T> left, BinaryTreeNode<T> right) => !(left == right);

    public static bool operator <(BinaryTreeNode<T> left, BinaryTreeNode<T> right) => left.Value.CompareTo(right.Value) < 0;
    public static bool operator >(BinaryTreeNode<T> left, BinaryTreeNode<T> right) => left.Value.CompareTo(right.Value) > 0;
}

This is the much easier to read BinaryTree

public class BinaryTree<T> where T : IComparable
{
    public BinaryTreeNode<T> Root { get; set; }

    //recursively insert new node into the bst instance with T data 
    public void Insert(T value, BinaryTreeNode<T> node)
    {
        var nodeToInsert = new BinaryTreeNode<T> { Value = value };

        if (nodeToInsert > node)
        {
            if (node.Right == null)
            {
                node.Right = nodeToInsert;
            }
            else
            {
                Insert(value, node.Right);
                Console.WriteLine("right");
            }
        }

        if (nodeToInsert < node)
        {
            if (node.Left == null)
            {
                node.Left = nodeToInsert;
            }
            else
            {
                Insert(value, node.Left);
                Console.WriteLine("left");
            }
        }
    }

    //normal insertion method (inserts T data into a bst instance)
    public void Insert(T value)
    {
        var nodeToInsert = new BinaryTreeNode<T> { Value = value };

        if (Root == null)
        {
            Root = nodeToInsert;
        }

        var current = Root;

        while (current != null)
        {
            if (nodeToInsert > current)
            {
                if (current.Right != null)
                {
                    current = current.Right;
                    continue;
                }
                current.Right = nodeToInsert;
                continue;
            }

            if (nodeToInsert < current)
            {
                if (current.Left != null)
                {
                    current = current.Left;
                    continue;
                }
                current.Left = nodeToInsert;
                continue;
            }

            return;
        }
    }

    //insert all elements from an array
    public void InsertRange(IEnumerable<T> source)
    {
        foreach (var item in source)
        {
            Insert(item);
        }
    }
    //get the last level in the bst instance
    public int GetLevel(BinaryTreeNode<T> node, int current = 1)
    {
        int right = 0;
        int left = 0;

        if (node.Right != null)
        {
            right = GetLevel(node.Right, current + 1);
        }

        if (node.Left != null)
        {
            left = GetLevel(node.Right, current + 1);
        }

        if (right == 0 && left == 0) return current; //this is readable in my opinion but according to best practices (microsoft), only one operation per line is good, so should this be changed or is it okay

        else
        {
            return right > left ? right : left; //is this acceptable?
        }

    }
    
    //get a queue of all the values in a given level
    public void GetOnLevel(BinaryTreeNode<T> node, int curLevel, int trgLevel, Queue<T> result)
    {
        if (curLevel == trgLevel)
        {
            result.Enqueue(node.Value);
        }
        else
        {
            if (node.Left != null)
            {
                GetOnLevel(node.Left, curLevel + 1, trgLevel, result);
            }

            if (node.Right != null)
            {
                GetOnLevel(node.Right, curLevel + 1, trgLevel, result);
            }
        }
    }
}

And finally this is the new BinaryTreeSearch

public class BinaryTreeSearch
{
    //returns the node that holds data equivalent to T data
    public BinaryTreeNode<T> FindNode<T>(T value, BinaryTreeNode<T> source) where T : IComparable
    {
        if (source == null) return null;
        
        var nodeToFind = new BinaryTreeNode<T> { Value = value };

        if (nodeToFind == source)
        {
            return source;
        }

        if (nodeToFind > source)
        {
            return FindNode<T>(value, source.Right);
        }
        
        if (nodeToFind < source)
        {
            return FindNode<T>(value, source.Left);
        }

        return null;
    }
}

I hope I didn't broke anything but it should be more of an example rather then fully tested code.