Return to Revisions

Using recursion in this instance helps a lot to reduce code duplication. Particularly, in this case, you have different code blocks depending on whether left() or right() are null.

Using recursion you only need one null check, and this removes a big chunk of code.

The queue is only needed for recursion as well, and, it's purpose was hard to figure out... you should have documented it more carefully...

The map variable is also a variable which has a poorly documented purpose... it's hard to tell what variables are on that, and why.

All in all, your suspicions are right that there's a better way to do this.....

Algorithmically it strikes me that a single Map<List<Integer>> is probably the right structure, and that recursion is a better solution.... consider a recursive function:

recurseTraverse(final Node<Integer> node, final Map<Integer, List<Node<Integer>>> columnmap, final int column) {
    if (node == null) {
        return;
    }
    List<Node<Integer>> list = columnmap.get(column);
    if (list == null) {
        list = new ArrayList<Node<Integer>>();
        columnmap.put(column, list);
    }
    list.add(node);
    recurse(node.left(), columnmap, column - 1);
    recurse(node.right(), columnmap, column + 1);

}

Then calling that with a TreeMap<Integer, List<Node<Integer>>> you should be able to get an improved version of your result.

public void traverse(Node<Integer> root) {
    TreeMap<Integer, List<Node<Integer>>> columnMap = new TreeMap<>();

    recurseTraverse(root, columnMap, 0);

    for (Entry<Integer, List<Node<Integer>>> entry: columnMap.entrySet()) {
        System.out.println("Column - " + entry.getKey() + " : " + entry.getValue());
    }
}