Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove method fails for single / root key #4

Open
codelinter opened this issue Apr 11, 2015 · 5 comments
Open

Remove method fails for single / root key #4

codelinter opened this issue Apr 11, 2015 · 5 comments
Labels

Comments

@codelinter
Copy link

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

After calling Remove, the trie still finds the key.

@derekparker
Copy link
Owner

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

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

@derekparker derekparker changed the title Remove method is not working Remove method fails for single / root key Apr 11, 2015
@codelinter
Copy link
Author

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{}
            }
        }

    }

}

@derekparker
Copy link
Owner

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.

@rench1988
Copy link

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)

@jamierobertsusa
Copy link
Contributor

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants