Return to Answer

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.

##Example

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.

Example

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.