Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Copy link
Copy Markdown

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
Copy Markdown
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
}
49 changes: 49 additions & 0 deletions linkedhashset_benchmark_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package set

import "testing"

func BenchmarkLinkedHashSet_Contains_vs_InArray(b *testing.B) {
set := NewLinkedHashSet[string]()
set.Add(giantGenericSlice...)

foundTarget := giantGenericSlice[len(giantGenericSlice)/2]
notFoundTarget := "___not_present___"

b.Run("Found", func(b *testing.B) {
b.Run("Contains", func(b *testing.B) {
b.ReportAllocs()
var sink bool
for i := 0; i < b.N; i++ {
sink = set.Contains(foundTarget)
}
_ = sink
})
b.Run("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", func(b *testing.B) {
b.Run("Contains", func(b *testing.B) {
b.ReportAllocs()
var sink bool
for i := 0; i < b.N; i++ {
sink = set.Contains(notFoundTarget)
}
_ = sink
})
b.Run("InArray", func(b *testing.B) {
b.ReportAllocs()
var sink bool
for i := 0; i < b.N; i++ {
sink = set.InArray(notFoundTarget)
}
_ = sink
})
})
}
29 changes: 29 additions & 0 deletions linkedhashset_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -196,3 +196,32 @@ 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")
println(set.linkedHashMap.table)
println(set.linkedHashMap.Get("02")) // debug
Comment thread
fm4teus marked this conversation as resolved.
Outdated

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"))
})
}