HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Binary Search Tree insert method (map interface)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mapinsertsearchmethodinterfacebinarytree

Problem

This is my implementation based on Map interface. The BST is a linked binary tree with root reference, both internal and external nodes (MyBSNode) contains (key, value) entries

What do you think? There's something to improve?

public BSPosition insert(int k, E e) {
    if(isEmpty()){
        /* Sets root */
        BSPosition r = new MyBSNode(null);
        r.setElement(k, e);
        setRoot(r);
        size++;
        return r;
    }

    BSPosition n = treeInsert(k, root);          //k-key node or last node visited
    if(k == n.getKey()){                            //tree has already a k-key node
        n.setElement(k, e);                         //only modifies e
        return n;           
    }
    /* n is a external (last visited) node with different key */
    size++;
    if(k  treeInsert(int k, BSPosition v){
    if(isLeaf(v))                       return v;           

    if(k < v.getKey()){
        if(v.getLeftChild() == null)    return v;
        return treeInsert(k, v.getLeftChild());
    }
    else if(k == v.getKey())            return v;

    else{
        if(v.getRightChild() == null)   return v;
        return treeInsert(k, v.getRightChild());
    }
}

Solution

The algorithm looks fine. Some other notes:

-
I would use longer variable names than r, k, e etc. Longer names would make the code more readable since readers/maintainers don't have to decode or memorize the abbreviations.

-

new MyBSNode(null)


Consider creating a default contructor, a named factory method or an explanatory local variable for null. Currently readers have to check the MyBSNode constructor to figure out what's that null supposed to mean.

-
MyBSNode does not seem to have a good name. Try to find something more descriptive.

-
n.setElement(k, e); // only modifies e


Creating a setValue(E value) method would make the comment unnecessary and would be cleaner.

-
I'd consider moving isLeaf to BSPosition since it seems data envy. I guess its only task is to check that both left and right children are nulls.

-
treeInsert does not do any insertion, its name is a little bit misleading, it just searches the parent of the new node or a node with the same key. I'd call it according to that. (searchInsertionPoint for example.)

Code Snippets

new MyBSNode<E>(null)
n.setElement(k, e); // only modifies e

Context

StackExchange Code Review Q#30519, answer score: 2

Revisions (0)

No revisions yet.