Solve Data Structure#2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements core functionality for multiple data structures including Binary Search Tree, Stack, Queue, HashSet, HashMap, Graph, and various LinkedList implementations. The implementation replaces panic("todo: please implement me!") stubs with working code for operations like insertion, deletion, traversal, and searching across all these data structures.
Changes:
- Implemented BST operations (Add, Del, Exists, Min, Max) with tree traversal methods (InOrder, PreOrder, PostOrder)
- Implemented Stack and Queue operations with backend initialization
- Implemented HashSet operations (Add, Del, Union, Intersection, Disjoint) and HashMap operations (Put, Del, Get, rehashing)
- Implemented Graph operations (AddEdge, DelEdge, HasEdge, vertex iteration)
- Implemented LinkedList operations for Singly, Doubly, and Circular variants (Append, Prepend, Pop, Shift, Insert, Remove)
- Implemented DynamicArray operations with automatic resizing
- Implemented BitSet operations for set manipulation
- Updated CI workflow to include "master" branch
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tree/binary_search_tree.go | Implements BST insertion, deletion, search, and traversal operations using recursive helpers |
| stack/stack.go | Implements stack operations with backend initialization and delegation to linked list |
| sets/hashset.go | Implements set operations including union, intersection, and disjoint checks using hashmap backend |
| queue/queue.go | Implements queue operations with backend initialization and delegation to linked list |
| linkedlist/singly_linkedlist.go | Implements singly linked list operations including insertion, deletion, and traversal |
| linkedlist/doubly_linkedlist.go | Implements doubly linked list operations with bidirectional traversal and optimized node finding |
| linkedlist/circular_doubly_linkedlist.go | Implements circular doubly linked list operations maintaining circular structure |
| hashmap/hashmap_chaining.go | Implements hash map with chaining collision resolution, including dynamic resizing |
| graph/graph.go | Implements graph operations using adjacency list representation for both directed and undirected graphs |
| dynamicarray/dynamicarray.go | Implements dynamic array with automatic capacity doubling and element manipulation |
| dynamicarray/dynamicarray_test.go | Adds EnumBackward test case |
| bitsets/bitsets.go | Implements bitset operations for efficient integer set representation |
| .github/workflows/ci.yml | Updates CI to trigger on both "master" and "main" branches |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for i, v := range sequence.Enum(list.Iter) { | ||
| if v == from { | ||
| list.Remove(i) | ||
| } |
There was a problem hiding this comment.
The same logic error exists here: after removing an edge with list.Remove(i), the loop continues without a break statement. This can cause incorrect behavior if there are multiple edges or lead to index errors. Add a break statement after list.Remove(i).
| d.Append(value) | ||
| for i := d.size - 1; i > 0; i-- { | ||
| d.Swap(i, i-1) | ||
| } | ||
| d.backend.Set(0, value) | ||
| // d.size++ |
There was a problem hiding this comment.
The Prepend method has inefficient logic. It calls Append(value) first which adds the value to the end and increments size, then swaps elements to move it to the front, but never decrements size. This means size is incorrectly incremented twice. The commented line "// d.size++" at line 301 confirms this issue. Instead of using Append, the implementation should check capacity, grow if needed, shift elements right, set value at index 0, and increment size once.
| d.Append(value) | |
| for i := d.size - 1; i > 0; i-- { | |
| d.Swap(i, i-1) | |
| } | |
| d.backend.Set(0, value) | |
| // d.size++ | |
| // Place the new value at the new logical end, then move it to the front. | |
| d.backend.Set(d.size, value) | |
| d.size++ | |
| for i := d.size - 1; i > 0; i-- { | |
| d.Swap(i, i-1) | |
| } |
| result := New(b.Len()) | ||
| for i, _ := range result.bitfields { |
There was a problem hiding this comment.
The Disjoint method unnecessarily creates a new BitSet result but never uses it for the intended purpose. The result variable is initialized but only used to determine loop range. The variable should be removed as it wastes memory and serves no purpose in the logic.
| result := New(b.Len()) | |
| for i, _ := range result.bitfields { | |
| for i := range b.bitfields { |
| panic("BitSets.Union: The length unmatch") | ||
| } | ||
| result := New(b.Len()) | ||
| for i, _ := range result.bitfields { |
There was a problem hiding this comment.
Blank identifier should not be used in range loops when the value is not needed. Change "for i, _ := range result.bitfields" to "for i := range result.bitfields"
| for i, _ := range result.bitfields { | |
| for i := range result.bitfields { |
| panic("BitSets.Intersection: The length unmatch") | ||
| } | ||
| result := New(b.Len()) | ||
| for i, _ := range result.bitfields { |
There was a problem hiding this comment.
Blank identifier should not be used in range loops when the value is not needed. Change "for i, _ := range result.bitfields" to "for i := range result.bitfields"
| for i, v := range sequence.Enum(list.Iter) { | ||
| if v == to { | ||
| list.Remove(i) | ||
| } |
There was a problem hiding this comment.
The DelEdge method has a logic error: it removes all edges from 'from' to 'to', but the break statement is missing after list.Remove(i). Without a break, the iteration continues with incorrect indices after removal, and may remove multiple edges or cause index errors. Add a break statement after list.Remove(i) to exit the loop once the edge is found and removed.
| return shiftData, true | ||
| } else { | ||
| tail := l.head.prev | ||
| newHead := l.head.next |
There was a problem hiding this comment.
The TryShift method in CircularDoublyLinkedList doesn't properly rewire tail.next when removing the head in a multi-node list. After setting newHead and newHead.prev = tail, the code should also set tail.next = newHead to maintain the circular structure. Currently, tail still points to the old (removed) head.
| newHead := l.head.next | |
| newHead := l.head.next | |
| tail.next = newHead |
| // hint: if s.b == nil, initialize it with linkedlist.NewDoublyLinkedList[E]() | ||
| panic("todo: please implement me!") | ||
| if s.b == nil { | ||
| linkedlist.NewDoublyLinkedList[E]() |
There was a problem hiding this comment.
The ensureBackend function creates a new backend but doesn't assign it to s.b. The created backend is immediately discarded, so s.b remains nil. This will cause all stack operations to fail. The assignment should be: s.b = linkedlist.NewDoublyLinkedListE
| linkedlist.NewDoublyLinkedList[E]() | |
| s.b = linkedlist.NewDoublyLinkedList[E]() |
| // Have two children | ||
| successor, _ := t.Min() |
There was a problem hiding this comment.
In the deleteHelper method for a node with two children, the code incorrectly calls t.Min() to find the successor. This returns the minimum value of the entire tree (from the root), not the minimum value in the right subtree of the node being deleted. The correct approach is to find the minimum node in node.right's subtree. Consider adding a helper method that finds the minimum starting from a given node, or traverse node.right to its leftmost descendant.
| // Have two children | |
| successor, _ := t.Min() | |
| // Have two children: find the minimum in the right subtree (successor) | |
| successorNode := node.right | |
| for successorNode.left != nil { | |
| successorNode = successorNode.left | |
| } | |
| successor := successorNode.data |
| return t.existsHelper(t.root, value) | ||
| } | ||
|
|
||
| // Recursive helper for `Exist` method |
There was a problem hiding this comment.
The comment refers to 'Exist' method, but the actual method name is 'Exists'. The comment should say "Recursive helper for Exists method"
| // Recursive helper for `Exist` method | |
| // Recursive helper for `Exists` method |
No description provided.