Add solution for Challenge 27 by Johrespi#723
Conversation
WalkthroughAdds a Go generics utilities module providing Pair, Stack, Queue, Set types plus common slice utilities and a shared ErrEmptyCollection error for empty-collection operations (constructors, accessors, mutators, and set operations included). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant G as generics package
rect rgb(240,248,255)
note over C,G: Create and operate on collections
C->>G: NewStack[T]()
C->>G: Push(value)
C->>G: Pop()
alt empty
G-->>C: ErrEmptyCollection
else not empty
G-->>C: value
end
end
rect rgb(245,255,250)
C->>G: NewQueue[T]()
C->>G: Enqueue(value)
C->>G: Dequeue()
alt empty
G-->>C: ErrEmptyCollection
else not empty
G-->>C: value
end
end
rect rgb(255,250,240)
C->>G: NewSet[T]()
C->>G: Add(v)
C->>G: Union(s1,s2)
G-->>C: *Set[T]
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
challenge-27/submissions/Johrespi/solution-template.go (1)
19-213: Clean up placeholder TODO comments.The functions are already implemented, so the remaining
// TODOmarkers are misleading. Please remove them (and similar ones throughout the file) to avoid implying unfinished work.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
challenge-27/submissions/Johrespi/solution-template.go (1)
245-258: Handle nil set inputs.The
Intersectionfunction will panic if eithers1ors2is a zero-value Set with nil maps. Add nil checks before ranging.func Intersection[T comparable](s1, s2 *Set[T]) *Set[T] { - // TODO: Implement this function - s3 := NewSet[T]() - for k, _ := range s1.items { - if s2.Contains(k) { - s3.Add(k) + if s1 != nil && s1.items != nil && s2 != nil { + for k := range s1.items { + if s2.Contains(k) { + s3.Add(k) + } } } return s3 }
🧹 Nitpick comments (4)
challenge-27/submissions/Johrespi/solution-template.go (4)
88-96: Simplify to idiomatic Go.The IsEmpty method can be simplified to a single return statement.
func (s *Stack[T]) IsEmpty() bool { - // TODO: Implement this method - - if len(s.items) == 0 { - return true - } - - return false + return len(s.items) == 0 }
157-163: Simplify to idiomatic Go.The IsEmpty method can be simplified to a single return statement.
func (q *Queue[T]) IsEmpty() bool { - // TODO: Implement this method - if len(q.items) == 0 { - return true - } - return false + return len(q.items) == 0 }
206-210: Guard zero-value Set from panics.Calling
Sizeon a zero-value Set returns 0 without panicking (sincelen(nil)is 0), but for defensive programming and consistency with other methods, consider adding an explicit nil check.func (s *Set[T]) Size() int { - // TODO: Implement this method + if s.items == nil { + return 0 + } return len(s.items) }
339-355: Simplify boolean map check.The comparison at line 347 is verbose. In Go, map lookups return false for missing keys, so you can check the boolean value directly.
for _, e := range slice { - if m[e] == true { + if m[e] { continue } res = append(res, e) m[e] = true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-27/submissions/Johrespi/solution-template.go(1 hunks)
🔇 Additional comments (10)
challenge-27/submissions/Johrespi/solution-template.go (10)
5-6: LGTM!The sentinel error pattern is correctly implemented and provides clear semantics for empty collection operations.
12-28: LGTM!The Pair implementation is correct with proper generic type handling and the Swap method correctly inverts the type parameters.
34-79: LGTM!The Stack implementation correctly handles LIFO semantics with proper empty-collection error handling.
181-188: LGTM!The nil check for zero-value Set correctly addresses the previous review feedback.
278-289: LGTM!The Filter implementation correctly applies the predicate and builds the result slice.
291-300: LGTM!The Map implementation correctly applies the mapper function to transform elements.
302-311: LGTM!The Reduce implementation correctly accumulates values using the reducer function.
313-324: LGTM!The Contains implementation correctly checks for element presence in the slice.
326-337: LGTM!The FindIndex implementation correctly returns the index of the first occurrence or -1 if not found.
212-226: Guard zero-value Set from panics.Calling
Elementson a zero-value Set panics when ranging overs.itemsat line 221. Please add a nil check.func (s *Set[T]) Elements() []T { - // TODO: Implement this method - - if len(s.items) == 0 { + if s.items == nil || len(s.items) == 0 { return []T{} }Likely an incorrect or invalid review comment.
| // Dequeue removes and returns the front element from the queue | ||
| // Returns an error if the queue is empty | ||
| func (q *Queue[T]) Dequeue() (T, error) { | ||
| // TODO: Implement this method | ||
| var zero T | ||
|
|
||
| if len(q.items) == 0 { | ||
| return zero, ErrEmptyCollection | ||
| } | ||
|
|
||
| removed := q.items[0] | ||
| q.items = q.items[1:] | ||
|
|
||
| return removed, nil | ||
| } |
There was a problem hiding this comment.
Memory leak: backing array retains removed elements.
The slice reslicing at line 131 (q.items = q.items[1:]) doesn't release the memory of dequeued elements. The backing array still holds references to removed items, preventing garbage collection. For long-lived queues, this can accumulate substantial memory.
Apply this diff to nil out the removed element before reslicing:
removed := q.items[0]
+ q.items[0] = zero // Release reference for GC
q.items = q.items[1:]Alternatively, for high-throughput queues, consider implementing a ring buffer or periodic slice reallocation when the queue becomes empty.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Dequeue removes and returns the front element from the queue | |
| // Returns an error if the queue is empty | |
| func (q *Queue[T]) Dequeue() (T, error) { | |
| // TODO: Implement this method | |
| var zero T | |
| if len(q.items) == 0 { | |
| return zero, ErrEmptyCollection | |
| } | |
| removed := q.items[0] | |
| q.items = q.items[1:] | |
| return removed, nil | |
| } | |
| // Dequeue removes and returns the front element from the queue | |
| // Returns an error if the queue is empty | |
| func (q *Queue[T]) Dequeue() (T, error) { | |
| // TODO: Implement this method | |
| var zero T | |
| if len(q.items) == 0 { | |
| return zero, ErrEmptyCollection | |
| } | |
| removed := q.items[0] | |
| q.items[0] = zero // Release reference for GC | |
| q.items = q.items[1:] | |
| return removed, nil | |
| } |
🤖 Prompt for AI Agents
In challenge-27/submissions/Johrespi/solution-template.go around lines 120 to
134, the Dequeue implementation reslices with q.items = q.items[1:] which leaves
the removed element referenced in the backing array and prevents GC; before
reslicing, assign the removed slot to the zero value (e.g., q.items[0] = zero)
to drop the reference, then reslice; for high-throughput scenarios consider
switching to a ring buffer or periodically copying remaining items into a new
slice when the head index grows large.
| // Remove removes an element from the set if it exists | ||
| func (s *Set[T]) Remove(value T) { | ||
| // TODO: Implement this method | ||
| delete(s.items, value) | ||
| } |
There was a problem hiding this comment.
Guard zero-value Set from panics.
Calling Remove on a zero-value Set panics because s.items is nil, similar to the issue fixed in Add. For consistency and safety, please initialize the map on demand.
func (s *Set[T]) Remove(value T) {
- // TODO: Implement this method
+ if s.items == nil {
+ return
+ }
delete(s.items, value)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Remove removes an element from the set if it exists | |
| func (s *Set[T]) Remove(value T) { | |
| // TODO: Implement this method | |
| delete(s.items, value) | |
| } | |
| // Remove removes an element from the set if it exists | |
| func (s *Set[T]) Remove(value T) { | |
| if s.items == nil { | |
| return | |
| } | |
| delete(s.items, value) | |
| } |
🤖 Prompt for AI Agents
In challenge-27/submissions/Johrespi/solution-template.go around lines 190 to
194, calling Remove on a zero-value Set can panic because s.items may be nil;
guard against this by checking if s.items == nil and initializing it with
make(map[T]struct{}) before performing the delete so Remove becomes safe to call
on zero-value sets.
| // Contains returns true if the set contains the given element | ||
| func (s *Set[T]) Contains(value T) bool { | ||
| // TODO: Implement this method | ||
| _, exists := s.items[value] | ||
| if exists { | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Guard zero-value Set from panics.
Calling Contains on a zero-value Set panics because s.items is nil. Please add a nil check and simplify the return logic.
func (s *Set[T]) Contains(value T) bool {
- // TODO: Implement this method
- _, exists := s.items[value]
- if exists {
- return true
- }
- return false
+ if s.items == nil {
+ return false
+ }
+ return s.items[value]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Contains returns true if the set contains the given element | |
| func (s *Set[T]) Contains(value T) bool { | |
| // TODO: Implement this method | |
| _, exists := s.items[value] | |
| if exists { | |
| return true | |
| } | |
| return false | |
| } | |
| // Contains returns true if the set contains the given element | |
| func (s *Set[T]) Contains(value T) bool { | |
| if s.items == nil { | |
| return false | |
| } | |
| return s.items[value] | |
| } |
🤖 Prompt for AI Agents
In challenge-27/submissions/Johrespi/solution-template.go around lines 196 to
204, Contains panics when called on a zero-value Set because s.items can be nil;
add a guard that returns false if s is nil or s.items is nil, and simplify the
logic to directly return the lookup result (e.g., perform the nil checks then
return exists).
| // Union returns a new set containing all elements from both sets | ||
| func Union[T comparable](s1, s2 *Set[T]) *Set[T] { | ||
| // TODO: Implement this function | ||
|
|
||
| s3 := NewSet[T]() | ||
|
|
||
| for k, _ := range s1.items { | ||
| s3.Add(k) | ||
| } | ||
|
|
||
| for k, _ := range s2.items { | ||
| s3.Add(k) | ||
| } | ||
|
|
||
| return s3 | ||
| } |
There was a problem hiding this comment.
Handle nil set inputs.
The Union function will panic if either s1 or s2 is a zero-value Set with nil maps. Add nil checks before ranging over the maps.
func Union[T comparable](s1, s2 *Set[T]) *Set[T] {
- // TODO: Implement this function
-
s3 := NewSet[T]()
- for k, _ := range s1.items {
- s3.Add(k)
+ if s1 != nil && s1.items != nil {
+ for k := range s1.items {
+ s3.Add(k)
+ }
}
- for k, _ := range s2.items {
- s3.Add(k)
+ if s2 != nil && s2.items != nil {
+ for k := range s2.items {
+ s3.Add(k)
+ }
}
return s3
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Union returns a new set containing all elements from both sets | |
| func Union[T comparable](s1, s2 *Set[T]) *Set[T] { | |
| // TODO: Implement this function | |
| s3 := NewSet[T]() | |
| for k, _ := range s1.items { | |
| s3.Add(k) | |
| } | |
| for k, _ := range s2.items { | |
| s3.Add(k) | |
| } | |
| return s3 | |
| } | |
| // Union returns a new set containing all elements from both sets | |
| func Union[T comparable](s1, s2 *Set[T]) *Set[T] { | |
| s3 := NewSet[T]() | |
| if s1 != nil && s1.items != nil { | |
| for k := range s1.items { | |
| s3.Add(k) | |
| } | |
| } | |
| if s2 != nil && s2.items != nil { | |
| for k := range s2.items { | |
| s3.Add(k) | |
| } | |
| } | |
| return s3 | |
| } |
🤖 Prompt for AI Agents
In challenge-27/submissions/Johrespi/solution-template.go around lines 228 to
243, the Union function ranges over s1.items and s2.items which will panic if s1
or s2 is a zero-value or has a nil items map; add nil-safe guards before
iterating: check if s1 != nil && s1.items != nil before the first range and
similarly for s2, so the function still returns a new empty set when either
input is nil without causing a runtime panic.
| // Difference returns a new set with elements in s1 that are not in s2 | ||
| func Difference[T comparable](s1, s2 *Set[T]) *Set[T] { | ||
| // TODO: Implement this function | ||
|
|
||
| s3 := NewSet[T]() | ||
|
|
||
| for k, _ := range s1.items { | ||
| if !s2.Contains(k) { | ||
| s3.Add(k) | ||
| } | ||
| } | ||
| return s3 | ||
| } |
There was a problem hiding this comment.
Handle nil set inputs.
The Difference function will panic if either s1 or s2 is a zero-value Set with nil maps. Add nil checks before ranging.
func Difference[T comparable](s1, s2 *Set[T]) *Set[T] {
- // TODO: Implement this function
-
s3 := NewSet[T]()
- for k, _ := range s1.items {
- if !s2.Contains(k) {
- s3.Add(k)
+ if s1 != nil && s1.items != nil && s2 != nil {
+ for k := range s1.items {
+ if !s2.Contains(k) {
+ s3.Add(k)
+ }
}
}
+
return s3
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Difference returns a new set with elements in s1 that are not in s2 | |
| func Difference[T comparable](s1, s2 *Set[T]) *Set[T] { | |
| // TODO: Implement this function | |
| s3 := NewSet[T]() | |
| for k, _ := range s1.items { | |
| if !s2.Contains(k) { | |
| s3.Add(k) | |
| } | |
| } | |
| return s3 | |
| } | |
| // Difference returns a new set with elements in s1 that are not in s2 | |
| func Difference[T comparable](s1, s2 *Set[T]) *Set[T] { | |
| s3 := NewSet[T]() | |
| if s1 != nil && s1.items != nil && s2 != nil { | |
| for k := range s1.items { | |
| if !s2.Contains(k) { | |
| s3.Add(k) | |
| } | |
| } | |
| } | |
| return s3 | |
| } |
🤖 Prompt for AI Agents
In challenge-27/submissions/Johrespi/solution-template.go around lines 260 to
272, the Difference function can panic when s1 or s2 are zero-value sets with
nil maps; add nil checks at the top: if s1 == nil || s1.items == nil return
NewSet[T]() (nothing to iterate), and if s2 == nil || s2.items == nil set s2 =
NewSet[T]() (treat as empty set) before the for-range so you never range over a
nil map and behavior matches treating nil as an empty set.
2 participants
Challenge 27 Solution
Submitted by: @Johrespi
Challenge: Challenge 27
Description
This PR contains my solution for Challenge 27.
Changes
challenge-27/submissions/Johrespi/solution-template.goTesting
Thank you for reviewing my submission! 🚀