trie icon indicating copy to clipboard operation
trie copied to clipboard

Remove method fails for single / root key

Open beyondgrayzone opened this issue 10 years ago • 5 comments

See this http://play.golang.org/p/of8h4uJ5jR

After calling Remove, the trie still finds the key.

beyondgrayzone avatar Apr 11 '15 01:04 beyondgrayzone

It seems if you only have one key, the remove fails.

Good catch, I'll push a fix up soon.

derekparker avatar Apr 11 '15 02:04 derekparker

I slightly modified the code with else , which seems to solve this micro bug

func (t *Trie) Remove(key string) {

    var (
        i int

        rs = []rune(key)

        node = t.nodeAtPath(key)
    )

    t.size--

    for n := node.Parent(); n != nil; n = n.Parent() {
        // fmt.Printf("%T \n", n)
        i++

        if len(n.Children()) > 1 {

            r := rs[len(rs)-i]

            n.RemoveChild(r)

            break

        } else {
            if t.root != nil {
                t.root = &Node{}
            }
        }

    }

}

beyondgrayzone avatar Apr 11 '15 16:04 beyondgrayzone

I think I'd change the above to something like:

if n == t.root {
        t.root = &Node{}
}

if len(....) {
...

If you wouldn't mind putting together a PR and adding a test to catch the bug I'd definitely pull it in.

derekparker avatar Apr 12 '15 21:04 derekparker

func (t *Trie) Remove(key string) {
    var (
        i    int
        rs   = []rune(key)
        node = findNode(t.Root(), []rune(key))
    )
    t.size--
    for n := node.Parent(); n != nil; n = n.Parent() {
        i++
        if len(n.Children()) > 1 {
            r := rs[len(rs)-i]
            n.RemoveChild(r)
            break
        }
    }
}

this will core dump when node is nil(key that isn't exist will crash)

rench1988 avatar Jan 22 '16 02:01 rench1988

Derek -- I will put a PR together for this 1 - Remove panics if key doesn't exist 2 - Remove doesn't work on root key Do you want me to change the method signature to return bool ?

jamierobertsusa avatar Aug 03 '16 22:08 jamierobertsusa