Can someone code review my "Distance between nodes in a BT"?

  • Context:
  • Thread starter Thread starter SlurrerOfSpeech
  • Start date Start date
  • Tags Tags
    Code Nodes Review
Click For Summary

Discussion Overview

The discussion centers around a code review for a solution to calculate the distance between nodes in a binary tree. Participants provide feedback on the code's organization, clarity, and documentation, focusing on its readiness for review and potential improvements.

Discussion Character

  • Technical explanation
  • Debate/contested

Main Points Raised

  • One participant notes that the code could be improved by removing a specific line that checks for a null ancestor, suggesting it would not affect functionality.
  • Another participant requests clarification on the intended functionality of the code, indicating a lack of context for the review.
  • A detailed review is provided, emphasizing the need for more comments throughout the code, including a summary of the class's purpose and descriptions for each method's inputs and outputs.
  • The reviewer critiques the class name "Solution" as inadequate and suggests that variable names should be more descriptive to enhance readability.
  • The reviewer stresses the importance of writing code that is easily understandable by others, especially in large codebases, and recommends formatting comments for documentation tools like Doxygen.

Areas of Agreement / Disagreement

Participants generally agree on the need for improved documentation and clarity in the code, but there is no consensus on specific changes or the overall quality of the solution.

Contextual Notes

The discussion highlights limitations in the code's documentation and organization, which may affect its comprehensibility and usability in larger projects.

SlurrerOfSpeech
Messages
141
Reaction score
11
Here's the solution I used for an online coding test with a major employer and I got rejected. Let me know about improvements I could make. Thanks.

Code:
using System;
using System.Collections.Generic;
using System.Linq;

class Node
{
    public int Val { get; set; }
    public Node Right { get; set; }   
    public Node Left { get; set; }
    public Node(int val) { Val = val; }
}public class Solution
{ 
    static Node _root = null;
   
    static void AddToTree(int val)
    {
        if(_root == null)
            _root = new Node(val);   
       
        for(Node cur = _root; ; )
        {   
            if(cur.Val < val)
            {
                if(cur.Right == null)
                {
                    cur.Right = new Node(val);
                    break;
                }
                else
                {
                    cur = cur.Right;
                }
            }
            else if(val < cur.Val)
            {
                if(cur.Left == null)
                {
                    cur.Left = new Node(val);
                    break;
                }
                else
                {
                    cur = cur.Left;
                }
            }
            else
            {
                break;
            }
        }
    }
   
   
    static Node NearestCommonAncestor(int i, int j)
    {
        int smaller = i < j ? i : j, 
           larger = i + j - smaller;
        return NearestCommonAncestor(smaller, larger, _root);
    }
   
    static Node NearestCommonAncestor(int i, int j, Node root)
    {
        if(root == null)
            return null;
        else if(root.Val < i)
            return NearestCommonAncestor(i, j, root.Right);
        else if(root.Val > j)
            return NearestCommonAncestor(i, j, root.Left);
        else
            return root;
    }

    static int DistanceToNode(Node start, int val)
    {
        int d = 0;
        for(Node cur = start; ; )
        {
            if(cur == null)
                return -1;
            else if(cur.Val < val)
            {
                cur = cur.Right;
                d += 1;
            }
            else if(cur.Val > val)
            {
                cur = cur.Left;
                d += 1;
            }
            else
                break;
        }
        return d;
    }
   
    static int DistanceBetweenNodes(int i, int j)
    {
        Node nca = NearestCommonAncestor(i, j);
        if(nca == null)
            return -1;
        int di = DistanceToNode(nca, i);
        int dj = DistanceToNode(nca, j);
        if(di == -1 || dj == -1)
            return -1;
        return di + dj;
    }
   
    public static void Main(String[] args)
    {
        int[] vals = { 2, 1, 5, 3, 8 };
        foreach(int i in vals)
            AddToTree(i);
        /*
                Now the tree should look like 
               
                2
              / \
             1   5
                / \
                3   8
                             
        */
        IEnumerable<string> results = 
            from x in vals
            from y in vals
            select string.Format
            (
                "Distance between {0} and {1} is {2}",
                x, y, DistanceBetweenNodes(x,y)
            );
        foreach(string message in results)
            Console.WriteLine(message);
    }
}
 
Technology news on Phys.org
Ah, I just realized that I could get rid of the

Code:
        if(nca == null)
            return -1;

line and it would run the same.
 
You might want to tell us what this code is supposed to do.
 
Here's my code review:
Without really studying your code, it seems like the overall organization and algorithm may be ok. That being said, it is not nearly ready for code review.
You need MANY more comments. At the top, give a top level summary of what the class is for. Each method needs a description of what it does, definitions of the inputs and outputs, any special comments about algorithms used. The class name "Solution" is totally inadequate for identifying what this stuff is. Anything tricky should have a comment about what is going on. Many variable names should be more descriptive of what they are.
I hate being so harsh, but I would not accept this code if someone gave it to me. I wouldn't even try to figure it out.

PS. Keep in mind that a company will have programs with millions of lines of code. And a programmer will spend most of his time diving into the middle of code that he did not program and doesn't have the time to figure out the whole thing with no help. Try to make your code such that another programmer can do that. You will be on the receiving end of code like that.

PPS. It is good practice to format your comments so that a documentation generator like Doxygen can use the top level ones you want it to use and will ignore the lower level detail ones.
 
Last edited:

Similar threads

  • · Replies 8 ·
Replies
8
Views
2K
  • · Replies 3 ·
Replies
3
Views
2K
Replies
1
Views
2K
  • · Replies 9 ·
Replies
9
Views
2K
  • · Replies 3 ·
Replies
3
Views
4K
  • · Replies 1 ·
Replies
1
Views
2K
  • · Replies 2 ·
Replies
2
Views
2K
  • · Replies 13 ·
Replies
13
Views
5K
  • · Replies 3 ·
Replies
3
Views
2K
  • · Replies 1 ·
Replies
1
Views
1K