Help me format small but ugly code

Getting Help Code Review
1 
func (n *Node) updateHeight() {
if n == nil {
	return
}

leftHeight := -1
if n.Left != nil {
	leftHeight = n.Left.Heigth
}

rightHeight := -1
if n.Right != nil {
	rightHeight = n.Right.Heigth
}

n.Heigth = 1 + leftHeight

if n.Heigth < 1+rightHeight {
	n.Heigth = 1 + rightHeight
}
}

This code seems a bit bulky than needed. Any tips on how to write it differently ? It simply updates the height of a node in a binary tree, based on its children.

2

Does this help you?

func (tree *Tree) insert(val int) *Tree {
	if tree == nil {
		return &Tree{nil, val, 0, nil}
	}
	tree.Height++ // increase at every insert the height of the node.
	if tree.Value > val {
		tree.Left = tree.Left.insert(val)
		return tree
	}
	tree.Right = tree.Right.insert(val)
	return tree
}
3

Hi @kync,

You idea is good, thx for sharing it.

My method though is about updating the height field, and it will be used when inserting a node, but also in other sitautions (like delete).

Thus I need to focus on refactoring it for readability, any suggestion is welcomed.

4

how about this

func (n *Node) updateHeight() {
	if n == nil {
		return
	}
	h := -1
	if n.Left != nil {
		h = n.Left.Height
	}
	if n.Right != nil && h.Right.Height > h {
		h = n.Right.Height
	}
	n.Height = h + 1
}
1 Like
5

Hi @root666

Best version so far, I like it a lot. Just wondering though, instead of calling the auxiliary variable h, I would call it maxChildHeight.

In reality I rarely call an auxiliary variable it with a large name, just to explain what it does, but in this case I feel it would help make understanding the method more easily.

What is your opinion on this ?

6

What about:

func (n *Node) Height() int {
	if n == nil {
		return -1
	}
	return max(n.Left.Height(), n.Right.Height()) + 1
}

func max(a, b int) int {
	if a > b {
		return a
	}
	return b
}

Now you do not need to updateHeight(), just use Height().

7

read this article and you will know my opinion :yum:

1 Like
8

Hi @j-forster,

Much more concise.

By the way, how do you deal with the max function being used in multiple packages in your porject, (similar to the contains(s string, slice []string).

Do you make some helper package, or simply have a copy of this method everywhere you use it ?

9

A common practice is to have a tools package containing often used internal functions like this one.

1 Like
10

Will switch works? You have multi-dimensional conditions.

func (n *Node) updateHeight() {
        switch {
        case n.Left == nil && n.Right == nil: 
                n.Height = 0
        case n.Left == nil && n.Right != nil:
                n.Height = n.Right.Height
        case n.Left != nil && n.Right == nil:
                fallthrough
        case n.Left.Height > n.Right.Height:
                n.Height = n.Left.Height
        default:
                n.Height = n.Right.Height
        }
}

note: order of each cases is important.

tested on: Go Playground - The Go Programming Language

1 Like