Feat: Understanding data structure#1
Conversation
glennsteven
force-pushed
the
glnsteven/ds
branch
from
e5d882b to
fded912
Compare
There was a problem hiding this comment.
Pull request overview
This pull request implements TODO functions across multiple data structure packages, converting placeholder panic statements into working implementations. The PR covers fundamental operations for stacks, queues, sets, linked lists (singly, doubly, and circular doubly), hashmaps, and dynamic arrays.
Changes:
- Implemented core data structure methods (Empty, Size, Push/Pop, Enqueue/Dequeue, Add/Del, Append/Prepend, etc.)
- Added iterator functions for various traversal patterns
- Implemented set operations (Union, Intersection, Disjoint)
- Added a redundant t.Helper() call in test utility
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| stack/stack.go | Implemented stack operations: Empty, Size, TryPeek, Push, TryPop, String, Iter, and backend initialization |
| queue/queue.go | Implemented queue operations: Empty, Size, TryPeek, Enqueue, TryDequeue, String, Iter, and backend initialization |
| sets/hashset.go | Implemented set operations: Add, Del, Exists, String, Iter, Union, Intersection, and Disjoint |
| linkedlist/singly_linkedlist.go | Implemented singly linked list operations: Append, Prepend, TryPop, TryShift, iterators, TryGet, TrySet, TryRemove, and Insert |
| linkedlist/doubly_linkedlist.go | Implemented doubly linked list operations: Prepend, Append, TryPop, TryShift, iterators, TryGet, TrySet, Insert, and TryRemove |
| linkedlist/circular_doubly_linkedlist.go | Implemented circular doubly linked list operations: Append, Prepend, TryPop, TryShift, TryGet, TrySet, Insert, TryRemove, and iterators |
| hashmap/hashmap_chaining.go | Implemented hashmap operations: Put, Del, internal put, Iter, Keys, Get, String, bucketIndex, and growAndRehash |
| dynamicarray/dynamicarray.go | Implemented dynamic array operations: Size, Cap, TryTail, Prepend, TryShift, Append, IterBackward, EnumBackward, TryRemove, plus formatting fixes |
| adt/prop/prop.go | Added redundant t.Helper() call in mustPanic test utility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| l.checkBounds(index) | ||
| curr := l.nodeAt(index) | ||
| n := NewBinaryNode(data, curr, curr.next) |
There was a problem hiding this comment.
Incorrect node creation: The second parameter should be curr.prev, not curr.next. According to the hint, the new node should have next=curr and prev=curr.prev. Currently, this creates a node with both next and prev pointing in the wrong direction for insertion.
| n := NewBinaryNode(data, curr, curr.next) | |
| n := NewBinaryNode(data, curr, curr.prev) |
| // 3) return (q.b.Head(), true) - front of queue is head | ||
| panic("todo: please implement me!") | ||
| q.ensureBackend() | ||
| if q.Empty() { |
There was a problem hiding this comment.
Inefficient: This calls q.Empty() which internally calls ensureBackend() again, resulting in redundant invocation. Should use q.b.Empty() instead to check the backend directly, following the same pattern as the stack implementation.
| if q.Empty() { | |
| if q.b.Empty() { |
| // 3) return (q.b.Shift(), true) - dequeue from front = shift from head | ||
| panic("todo: please implement me!") | ||
| q.ensureBackend() | ||
| if q.Empty() { |
There was a problem hiding this comment.
Inefficient: This calls q.Empty() which internally calls ensureBackend() again, resulting in redundant invocation. Should use q.b.Empty() instead to check the backend directly, following the same pattern as the stack implementation.
| if q.Empty() { | |
| if q.b.Empty() { |
| } | ||
| d.backend.Set(index, value) | ||
| return true | ||
| if d.Empty() || index < 0 || index >= d.Size() { |
There was a problem hiding this comment.
Redundant check: The d.Empty() condition is unnecessary because when the array is empty, d.Size() returns 0, and any valid index check index >= d.Size() will already handle that case. This is inconsistent with TryGet which only checks the index bounds.
| if d.Empty() || index < 0 || index >= d.Size() { | |
| if index < 0 || index >= d.Size() { |
| t.Helper() | ||
| defer func() { | ||
| if recover() == nil { | ||
| t.Helper() |
There was a problem hiding this comment.
Redundant: The t.Helper() call here is unnecessary. The function already calls t.Helper() on line 1460, which marks mustPanic as a test helper. Calling it again inside the deferred function doesn't provide additional value and is unusual Go testing practice.
| t.Helper() |
| } | ||
| h.size = nw.Size() | ||
| h.buckets = nw.buckets | ||
| nw = nil |
There was a problem hiding this comment.
Unnecessary assignment: Setting nw = nil is redundant since the variable goes out of scope immediately after this line. Go's garbage collector will handle the cleanup automatically once there are no more references to the old hashmap.
| nw = nil |
| l.head = n | ||
| } else { | ||
| n.next = l.head | ||
| l.head = n | ||
| } |
There was a problem hiding this comment.
Redundant assignment: n.next is already set to l.head during node creation on line 283 using NewUnaryNode(data, l.head). This line unnecessarily sets it again.
| l.head = n | |
| } else { | |
| n.next = l.head | |
| l.head = n | |
| } | |
| } | |
| l.head = n |
No description provided.