Add solutions for Challenges 1-7 by shahzodshafizod#1466
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds four new Go solutions: an integer Sum with overflow detection, a string ReverseString, a thread-safe Employee/Manager implementation, and a concurrent BFS query processor using a worker-pool. Changes
Sequence DiagramsequenceDiagram
participant Main as Main / Caller
participant JobChan as Job Channel (queue)
participant Worker as Worker Goroutine
participant BFS as bfs Helper
participant ResultChan as Result Channel
Main->>JobChan: enqueue start nodes (queries)
Main->>Worker: spawn N workers
Worker->>JobChan: receive start node
Worker->>BFS: call bfs(graph, start)
BFS->>BFS: traverse graph (BFS)
BFS-->>Worker: return visitation order
Worker->>ResultChan: send {start, order}
ResultChan-->>Main: deliver results
Main->>Main: aggregate into map[int][]int and return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 4
🧹 Nitpick comments (10)
challenge-2/submissions/shahzodshafizod/solution-template_test.go (2)
25-44: Consider testingReverseStringdirectly for faster and more reliable tests.The current approach spawns a subprocess for each test case, which adds overhead and external dependencies (requires
goin PATH). SinceReverseStringis exported and in the same package, you can test it directly:♻️ Suggested alternative using direct function call
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd := exec.Command("go", "run", "solution-template.go") - stdin := strings.NewReader(tt.input) - var stdout, stderr bytes.Buffer - cmd.Stdin = stdin - cmd.Stdout = &stdout - cmd.Stderr = &stderr - - err := cmd.Run() - if err != nil { - t.Fatalf("Error running the program: %v\nStderr: %s", err, stderr.String()) - } - - output := strings.TrimSpace(stdout.String()) + output := ReverseString(tt.input) if output != tt.expected { t.Errorf("For input '%s', expected output '%s', got '%s'", tt.input, tt.expected, output) } }) }This would also allow removing the
bytes,os/exec, andstringsimports.
16-23: Good test coverage; consider adding a Unicode/emoji test case.The test cases cover important scenarios well. Since the implementation uses
[]runeto handle Unicode correctly, consider adding a test case with multi-byte characters to validate this behavior:{"Special characters", "12345!@#$%", "%$#@!54321"}, {"Mixed case", "GoLang", "gnaLoG"}, + {"Unicode emoji", "Hello🌍🚀", "🚀🌍olleH"}, }challenge-1/submissions/shahzodshafizod/solution-template_test.go (1)
24-25: Duplicate test names cause unclear test output.Both overflow test cases are named "Overflow case", resulting in Go appending
#01to distinguish them in test output. Use distinct names for clarity.♻️ Proposed fix for distinct test names
- {"Overflow case", fmt.Sprintf("%d, %d", math.MaxInt, math.MaxInt), "errOverflow"}, - {"Overflow case", fmt.Sprintf("%d, %d", math.MinInt, math.MinInt), "errOverflow"}, + {"Positive overflow", fmt.Sprintf("%d, %d", math.MaxInt, math.MaxInt), "errOverflow"}, + {"Negative overflow", fmt.Sprintf("%d, %d", math.MinInt, math.MinInt), "errOverflow"},challenge-3/submissions/shahzodshafizod/solution-template.go (1)
26-28: Remove commented-out code.Dead code should be removed to keep the codebase clean. If the sorting logic is needed for reference, it can be recovered from version control.
challenge-3/submissions/shahzodshafizod/solution-template_test.go (2)
8-23: Consider adding sorted-order verification.The test verifies employee count but doesn't verify that employees remain sorted by ID. Given that
FindEmployeeByIDandRemoveEmployeerely on binary search, consider asserting the sorted invariant after adds.
1-5: Consider adding concurrency tests.The
Managerimplementation usessync.RWMutexfor thread safety, but there are no tests exercising concurrent Add/Remove/Find operations. Parallel tests usingt.Parallel()and goroutines would help validate the thread-safety guarantees.challenge-4/submissions/shahzodshafizod/solution-template.go (2)
16-17: Remove stale placeholder comments.Lines 16-17 still say the function is unimplemented; they now contradict the code and should be removed.
59-79: Trim non-essential scaffolding from the submission file.The commented-out alternative implementation and demo
main()add noise to the challenge solution and make review/maintenance harder.Also applies to: 101-117
challenge-4/submissions/shahzodshafizod/solution-template_test.go (2)
213-216: Turn warning-only timing checks into enforceable assertions.Both branches only
t.Logf(...), so sequential implementations can still pass these tests. If these checks are intended as requirements, switch tot.Errorf/t.Fatalfwith a stable tolerance policy.Also applies to: 266-269
120-123: Use unique-query cardinality for map-size assertions.
ConcurrentBFSQueriesreturnsmap[int][]int, so duplicate queries collapse to one key. Comparinglen(results)tolen(tc.queries)is fragile for future duplicate-query cases.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
challenge-1/submissions/shahzodshafizod/solution-template.gochallenge-1/submissions/shahzodshafizod/solution-template_test.gochallenge-2/submissions/shahzodshafizod/solution-template.gochallenge-2/submissions/shahzodshafizod/solution-template_test.gochallenge-3/submissions/shahzodshafizod/solution-template.gochallenge-3/submissions/shahzodshafizod/solution-template_test.gochallenge-4/submissions/shahzodshafizod/solution-template.gochallenge-4/submissions/shahzodshafizod/solution-template_test.go
challenge-1/submissions/shahzodshafizod/solution-template_test.go
Outdated
Show resolved
Hide resolved
challenge-3/submissions/shahzodshafizod/solution-template_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
challenge-3/submissions/shahzodshafizod/solution-template.go (1)
54-60:⚠️ Potential issue | 🟠 MajorAvoid returning a pointer to internal slice storage.
Line 59 returns
&m.Employees[index]; after unlock, concurrentappend/removecan invalidate that pointer and expose unsafe shared mutation.🐛 Proposed fix (keeps signature intact)
func (m *Manager) FindEmployeeByID(id int) *Employee { // O(n) m.mu.RLock() defer m.mu.RUnlock() for index := range m.Employees { if m.Employees[index].ID == id { - return &m.Employees[index] + e := m.Employees[index] + return &e } } return nil }
🧹 Nitpick comments (1)
challenge-4/submissions/shahzodshafizod/solution-template.go (1)
57-77: Remove the large commented-out implementation block.Keeping two implementations (one commented) in the submission file makes maintenance harder and increases drift risk.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
challenge-3/submissions/shahzodshafizod/solution-template.gochallenge-4/submissions/shahzodshafizod/solution-template.go
|
How can I merge this MR into main? |
shahzodshafizod
changed the title
shahzodshafizod
changed the title
shahzodshafizod
changed the title
2 participants
No description provided.