Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions linkedhashmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,21 +61,21 @@ func (l *linkedHashMap) Put(key, value interface{}) {
}

// Get gets an entry from the linked hash map
func (l *linkedHashMap) Get(key interface{}) interface{} {
func (l *linkedHashMap) Get(key interface{}) (value interface{}, found bool) {
hash := l.hash(key)
if _, ok := l.table[hash]; !ok {
return nil
return nil, false
}

tmp := l.table[hash]
for tmp != nil {
if tmp.key == key {
return tmp.value
return tmp.value, true
}
tmp = tmp.after
}

return nil
return nil, false
Comment on lines 70 to +78

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Olá! A mudança na assinatura do método Get é uma boa melhoria. No entanto, identifiquei um problema crítico de corretude e performance na implementação do linkedHashMap que invalida a premissa de complexidade O(1) para o Get e, consequentemente, para o novo método Contains.

  1. Tratamento de Colisão em Put: O método Put (não presente neste diff, mas parte do contexto) não trata colisões de hash. Ele simplesmente retorna se um hash já existe (if _, ok := l.table[hash]; ok { return }). Isso significa que se duas chaves diferentes produzirem o mesmo hash, a segunda chave e seu valor serão descartados, o que é um bug grave.

  2. Lógica de Busca em Get: O laço for (linhas 71-76) itera sobre a lista de ordem de inserção (tmp.after), não sobre uma cadeia de colisões. Isso é incorreto para resolver colisões e degrada a performance de O(1) para O(n) no pior caso, pois pode percorrer grande parte da lista.

Para que o Get (e o Contains) tenha a performance esperada, o linkedHashMap precisa ser corrigido para tratar colisões de hash adequadamente, por exemplo, usando encadeamento separado (separate chaining) para cada bucket no mapa.

Dado que este problema afeta diretamente o objetivo principal do seu PR, recomendo fortemente que a implementação do linkedHashMap seja corrigida.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tá certo, a intenção dessa lib é implementar uma estrutura de Conjunto, sem itens repetidos

}

// Remove removes an entry from the linked hash map
Expand Down
33 changes: 26 additions & 7 deletions linkedhashmap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,17 @@ func TestGet(t *testing.T) {
set := newLinkedHashMap()
value := rand.Int()
set.Put("test", value)
require.Equal(t, value, set.Get("test"))

v, exists := set.Get("test")
require.Equal(t, value, v)
require.True(t, exists)
})

t.Run("When the key not exists", func(t *testing.T) {
set := newLinkedHashMap()
result := set.Get("bla")
result, exists := set.Get("bla")
require.Nil(t, result)
require.False(t, exists)
})
}

Expand All @@ -27,7 +31,10 @@ func TestPut(t *testing.T) {
set := newLinkedHashMap()
value := rand.Int()
set.Put("test", value)
require.Equal(t, value, set.Get("test"))

v, exists := set.Get("test")
require.Equal(t, value, v)
require.True(t, exists)
})

t.Run("invalid key", func(t *testing.T) {
Expand All @@ -50,7 +57,10 @@ func TestRemove(t *testing.T) {

set.Remove(1)
require.Equal(t, 2, set.Length())
require.Nil(t, set.Get(1))

v, exists := set.Get(1)
require.Nil(t, v)
require.False(t, exists)
})

t.Run("last value", func(t *testing.T) {
Expand All @@ -61,7 +71,10 @@ func TestRemove(t *testing.T) {

set.Remove(3)
require.Equal(t, 2, set.Length())
require.Nil(t, set.Get(3))

v, exists := set.Get(3)
require.Nil(t, v)
require.False(t, exists)
})

t.Run("middle value", func(t *testing.T) {
Expand All @@ -72,14 +85,20 @@ func TestRemove(t *testing.T) {

set.Remove(2)
require.Equal(t, 2, set.Length())
require.Nil(t, set.Get(2))

v, exists := set.Get(2)
require.Nil(t, v)
require.False(t, exists)
})

t.Run("single value", func(t *testing.T) {
set := newLinkedHashMap()
set.Put(1, 1)
set.Remove(1)
require.Equal(t, 0, set.Length())
require.Nil(t, set.Get(1))

v, exists := set.Get(1)
require.Nil(t, v)
require.False(t, exists)
})
}
6 changes: 6 additions & 0 deletions linkedhashset.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ func (l *LinkedHashSet[T]) AsInterface() []interface{} {
}

// InArray returns whether the given item is in array or not
// DEPRECATED: use Contains method instead
func (l *LinkedHashSet[T]) InArray(search T) bool {
for item := range l.Iter() {
if item == search {
Expand All @@ -80,3 +81,8 @@ func NewLinkedHashSet[T comparable](items ...T) *LinkedHashSet[T] {
}
return lhm
}

func (l *LinkedHashSet[T]) Contains(search T) bool {
_, contains := l.linkedHashMap.Get(search)
return contains
}
55 changes: 55 additions & 0 deletions linkedhashset_benchmark_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package set

import (
"fmt"
"testing"
)

func BenchmarkLinkedHashSet_Contains_vs_InArray(b *testing.B) {
total := len(giantGenericSlice)
step := total / 5
sizes := []int{step, 2 * step, 3 * step, 4 * step, total}

for _, n := range sizes {
b.Run(fmt.Sprintf("N=%d", n), func(b *testing.B) {
set := NewLinkedHashSet[string]()
set.Add(giantGenericSlice[:n]...)

foundTarget := giantGenericSlice[n/2]
notFoundTarget := "___not_present___"

b.Run("Found/Contains", func(b *testing.B) {
b.ReportAllocs()
var sink bool
for i := 0; i < b.N; i++ {
sink = set.Contains(foundTarget)
}
_ = sink
})
b.Run("Found/InArray", func(b *testing.B) {
b.ReportAllocs()
var sink bool
for i := 0; i < b.N; i++ {
sink = set.InArray(foundTarget)
}
_ = sink
})
b.Run("NotFound/Contains", func(b *testing.B) {
b.ReportAllocs()
var sink bool
for i := 0; i < b.N; i++ {
sink = set.Contains(notFoundTarget)
}
_ = sink
})
b.Run("NotFound/InArray", func(b *testing.B) {
b.ReportAllocs()
var sink bool
for i := 0; i < b.N; i++ {
sink = set.InArray(notFoundTarget)
}
_ = sink
})
})
}
}
27 changes: 27 additions & 0 deletions linkedhashset_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -196,3 +196,30 @@ func TestLinkedHashSetAsInterface(t *testing.T) {
require.Equal(t, value.(testStruct), expectedArray[i])
}
}

func TestLinkedHashSetContains(t *testing.T) {
t.Run("found", func(t *testing.T) {
set := NewLinkedHashSet("02", "04", "06", "08")

require.True(t, set.Contains("02"))
require.True(t, set.Contains("04"))
require.True(t, set.Contains("06"))
require.True(t, set.Contains("08"))
})

t.Run("not found", func(t *testing.T) {
set := NewLinkedHashSet("02", "04", "06", "08")
require.False(t, set.Contains("01"))
require.False(t, set.Contains("03"))
require.False(t, set.Contains("05"))
require.False(t, set.Contains("07"))
})

t.Run("empty", func(t *testing.T) {
set := NewLinkedHashSet[string]()
require.False(t, set.Contains("01"))
require.False(t, set.Contains("03"))
require.False(t, set.Contains("05"))
require.False(t, set.Contains("07"))
})
}