Help me format small but ugly code


(Les Way) #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.


(Ali Koyuncu) #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
}

(Les Way) #3

Hi @ali_koyuncu,

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.


(Root Zhang) #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
}

(Les Way) #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 ?


(Johann Forster) #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().


(Root Zhang) #7

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


(Les Way) #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 ?


(Johann Forster) #9

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