Inserting a node in Binary Search Tree

I would like any suggestions and improvements for this code, be it generics or my Node class being completely mutable. Should I make it mutable? Is it good? Why?

package ishan.trees.tree;

class Node implements Comparable {

    private int data;
    public int getData() {
        return data;
    }

    public void setData(int data) {
        this.data = data;
    }

    public Node getLeftChild() {
        return leftChild;
    }

    public void setLeftChild(Node leftChild) {
        this.leftChild = leftChild;
    }

    public Node getRightChild() {
        return rightChild;
    }

    public void setRightChild(Node rightChild) {
        this.rightChild = rightChild;
    }

    private Node leftChild;
    private Node rightChild;

    public Node(int data,Node leftChild,Node rightChild)
    {
        this.data=data;
        this.leftChild=leftChild;
        this.rightChild=rightChild;

    }

    @Override
    public int compareTo(Node o) {
        if(o.getData() > this.data)
        return -1;

        if(o.getData() < this.data)
            return 1;

        return 0;
    }
}

Tree class

package ishan.trees.tree;

public class Tree {

    private Node root=null;

    public Node getRoot() {
        return root;
    }

    public void insertData(int data)
    {
        Node node=new Node(data,null,null);
        insert(node,this.root);

    }

    private Node insert(Comparable node,Node root1)
    {
            if(root1==null)
            {

                root1=new Node(((Node)node).getData(),null,null);
                if(this.root==null)
                {
                    this.root=root1;
                }
            }
            else if(node.compareTo(root1) 0)
            {

                root1.setLeftChild(insert(node,root1.getRightChild()));
            }

     return root1;  
    }
}

Generics

Since you asked: Sure, generics would be nice. Right now, your Tree would be better named IntBinarySearchTree, as it only accepts integers.

Mutable


Should I make it mutable? Is it good? Why?

No, mutable objects are not good. Very often, they are necessary (because the data changes after the object is created), but if you can make an object immutable, you should do so (because it's easier to handle if you know that it doesn't change).

In this case, I think that it is ok that it is mutable. You could change it, but the resulting code wouldn't be all that pretty, and it would get even worse when you implement more functionality.

Formatting

You are violating a lot of Java style conventions (opening { should be on same line, as should else; whitespace around ==, >, =; etc). If you paste your code into any IDE, it can format it according to conventions.

By convention, the order of elements in a class is: fields, constructor, methods. I would go with this order, so readers can easily find them.

private Node class

As your Node class is never used on its own, and never outside of the Tree class, it should be a private class inside Tree.

Node constructor

It's always a bit ugly to pass null to a constructor. I would create an additional constructor that only takes data as input.

compareTo

Your compareto method can be replaced with return this.data - o.getData();. It's a bit less readable, but it is how it's done most of the time, so readers will be used to it. You might also want to check if the passed node is not null, and if it is handle it accordingly.

insert

You should move the code for inserting the first root node to insertData. It just complicates insert.

I would also rename root1 to currentRoot.

And I would write a toString method for the tree to test it. Right now, your code doesn't seem quite right (you are doing root1.setLeftChild in both cases).

StackExchange Code Review Q#65265, answer score: 4