Add solution for Challenge 21 by krypton-io #636
Conversation
WalkthroughThis PR introduces a new Go file with three binary search functions: BinarySearch (iterative), BinarySearchRecursive (recursive), and FindInsertPosition (for maintaining sorted order). Each function operates on sorted integer slices with boundary optimizations and includes a main function demonstrating usage. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
🧹 Nitpick comments (2)
challenge-21/submissions/krypton-io/solution-template.go (2)
29-29: Remove TODO comments.The functions are implemented, so the TODO comments at lines 29, 70, and 105 should be removed.
Also applies to: 70-70, 105-105
69-100: Recursive implementation is correct.The binary search logic and mid calculation (
left + ((right - left) / 2)) are correct. The base case properly handles termination.Minor optimization: the empty array check at lines 72-75 executes on every recursive call but only needs to run once. Consider moving it outside or removing it since invalid indices would be caught by the base case.
| for start != mid { | ||
|
|
||
| if target == arr[mid] { | ||
| return mid | ||
| } | ||
|
|
||
| if target < arr[mid] { | ||
| end = mid | ||
| mid /= 2 | ||
| } else { | ||
| start = mid | ||
| mid = mid + ((end - mid) / 2) | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: incorrect mid calculation breaks binary search.
Line 57 uses mid /= 2 when narrowing the search to the left half. This calculation ignores the start position and can cause mid to fall below start, accessing elements outside the valid search range.
Example: searching for 55 in [10, 20, 30, 40, 50, 60, 70, 80]:
- start=0, end=7, mid=3 → arr[3]=40 < 55
- start=3, end=7, mid=5 → arr[5]=60 > 55
- end=5, mid=2 (via line 57: 5/2=2) → mid < start!
- Algorithm now accesses arr[2]=30, outside valid range [3,5]
Apply this diff:
for start != mid {
if target == arr[mid] {
return mid
}
if target < arr[mid] {
end = mid
- mid /= 2
+ mid = start + (end - start) / 2
} else {
start = mid
mid = mid + ((end - mid) / 2)
}
}📝 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.
| for start != mid { | |
| if target == arr[mid] { | |
| return mid | |
| } | |
| if target < arr[mid] { | |
| end = mid | |
| mid /= 2 | |
| } else { | |
| start = mid | |
| mid = mid + ((end - mid) / 2) | |
| } | |
| } | |
| for start != mid { | |
| if target == arr[mid] { | |
| return mid | |
| } | |
| if target < arr[mid] { | |
| end = mid | |
| mid = start + (end - start) / 2 | |
| } else { | |
| start = mid | |
| mid = mid + ((end - mid) / 2) | |
| } | |
| } |
🤖 Prompt for AI Agents
In challenge-21/submissions/krypton-io/solution-template.go around lines 49 to
62, the left-half branch sets mid via "mid /= 2", which ignores start and can
move mid below start; change the logic so after setting end = mid you recompute
mid relative to start, e.g. mid = start + (end - start)/2 (integer division),
ensuring mid stays within [start, end) and prevents out-of-range accesses.
| for start != mid { | ||
|
|
||
| if target == arr[mid] { | ||
| return mid | ||
| } | ||
|
|
||
| if target < arr[mid] { | ||
| end = mid | ||
| mid /= 2 | ||
| } else { | ||
| start = mid | ||
| mid = mid + ((end - mid) / 2) | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: incorrect mid calculation (same bug as BinarySearch).
Line 136 has the same critical bug as BinarySearch: mid /= 2 ignores the start position and can cause mid to fall below start, leading to incorrect array access.
Apply this diff:
for start != mid {
if target == arr[mid] {
return mid
}
if target < arr[mid] {
end = mid
- mid /= 2
+ mid = start + (end - start) / 2
} else {
start = mid
mid = mid + ((end - mid) / 2)
}
}🤖 Prompt for AI Agents
In challenge-21/submissions/krypton-io/solution-template.go around lines 128 to
141, the mid calculation is incorrect: when narrowing the search to the left
half the code does `mid /= 2`, which ignores `start` and can move mid below
start; replace both mid updates to compute the midpoint relative to start and
end (e.g., after setting end = mid compute mid = start + ((end - start) / 2),
and in the else branch after setting start = mid compute mid = start + ((end -
start) / 2)) so mid always stays within [start,end] and prevents invalid
accesses.
| if mid == 0 { | ||
| return 0 | ||
| } else { | ||
| if(target < arr[mid]) { | ||
| return mid - 1 | ||
| } else { | ||
| return mid + 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the insertion position logic.
The final adjustment returns mid - 1 when target < arr[mid] (line 147). This seems incorrect—if the target is less than the element at mid, it should typically be inserted at position mid, not mid - 1.
Please verify this logic is correct for all edge cases, especially when mid > 0.
Run the following test to verify the insertion position logic:
🏁 Script executed:
#!/bin/bash
# Test FindInsertPosition with various edge cases
cat > test_insert.go << 'EOF'
package main
import "fmt"
func FindInsertPosition(arr []int, target int) int {
length := len(arr)
start := 0
end := length - 1
mid := end / 2
if length == 0 {
return 0
}
if target > arr[end] {
return end + 1
}
if target == arr[start] {
return start
}
if target == arr[end] {
return end
}
for start != mid {
if target == arr[mid] {
return mid
}
if target < arr[mid] {
end = mid
mid /= 2
} else {
start = mid
mid = mid + ((end - mid) / 2)
}
}
if mid == 0 {
return 0
} else {
if(target < arr[mid]) {
return mid - 1
} else {
return mid + 1
}
}
}
func main() {
testCases := []struct{
arr []int
target int
expected int
}{
{[]int{1,3,5,7,9}, 4, 2},
{[]int{1,3,5,7,9}, 6, 3},
{[]int{1,3,5,7,9}, 2, 1},
{[]int{1,3,5,7,9}, 0, 0},
{[]int{2,4,6,8}, 5, 2},
{[]int{10,20,30,40,50}, 25, 2},
}
for _, tc := range testCases {
result := FindInsertPosition(tc.arr, tc.target)
status := "✓"
if result != tc.expected {
status = "✗"
}
fmt.Printf("%s Insert %d in %v: got %d, expected %d\n",
status, tc.target, tc.arr, result, tc.expected)
}
}
EOF
go run test_insert.go
rm test_insert.goLength of output: 374
Fix insertion position return values at lines 147 and 149.
The test confirms a bug: inserting 2 into [1,3,5,7,9] returns 0 instead of the correct position 1. The logic incorrectly returns mid - 1 when target < arr[mid], when it should return mid (the correct insertion point before the larger element). Similarly, line 149 should return mid instead of mid + 1.
if mid == 0 {
return 0
} else {
if(target < arr[mid]) {
return mid
} else {
return mid
}
}
🤖 Prompt for AI Agents
In challenge-21/submissions/krypton-io/solution-template.go around lines 143 to
151, the insertion-position logic is wrong: when mid != 0 the code returns mid-1
or mid+1 causing incorrect positions (e.g., inserting 2 into [1,3,5,7,9] yields
0). Change the two return statements on lines 147 and 149 so both return mid
(i.e., when mid != 0 always return mid), leaving the existing mid==0 return
as-is; you can also simplify by removing the inner if and just returning mid for
the non-zero case.
2 participants
Challenge 21 Solution
Submitted by: @krypton-io
Challenge: Challenge 21
Description
This PR contains my solution for Challenge 21.
Changes
challenge-21/submissions/krypton-io/solution-template.goTesting
Thank you for reviewing my submission! 🚀