Skip to content

Commit 6878ff8

Browse files
KentBeckona-agent
andcommitted
Refactor binary search into reusable find_key_index helper function
- Extract duplicated binary search logic from get() and insert() methods - Create find_key_index() helper that returns (index, found) tuple: * If found: index is position of existing key * If not found: index is insertion point for new key - Simplify get() method to 4 lines using helper - Simplify insert() method logic and improve readability - Add comprehensive test for find_key_index helper function Benefits: - DRY principle: eliminates code duplication - Single source of truth for binary search logic - Easier to maintain and debug - Consistent behavior across methods - More readable method implementations All existing tests still pass (20/20) New helper function test passes (1/1) Co-authored-by: Ona <no-reply@ona.com>
1 parent 63d3640 commit 6878ff8

File tree

1 file changed

+77
-52
lines changed

1 file changed

+77
-52
lines changed

rust/src/compressed_node.rs

Lines changed: 77 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -137,26 +137,17 @@ where
137137
unsafe fn set_value_at(&mut self, index: usize, value: V) {
138138
*self.values_ptr_mut().add(index) = value;
139139
}
140-
}
141140

142-
// Placeholder implementations - will be implemented through TDD
143-
impl<K, V> CompressedLeafNode<K, V>
144-
where
145-
K: Copy + Ord,
146-
V: Copy,
147-
{
148-
/// Insert a key-value pair into the leaf.
149-
/// Returns Ok(Some(old_value)) if key existed, Ok(None) if new key, Err if full.
150-
pub fn insert(&mut self, key: K, value: V) -> Result<Option<V>, &'static str> {
151-
// Check if we have space (unless it's an update)
152-
if self.len >= self.capacity {
153-
// Check if this is an update to existing key
154-
if self.get(&key).is_none() {
155-
return Err("Leaf is at capacity");
156-
}
141+
/// Find the index for a key using binary search.
142+
/// Returns (index, found) where:
143+
/// - If found: index is the position of the key
144+
/// - If not found: index is where the key should be inserted
145+
#[inline]
146+
fn find_key_index(&self, key: &K) -> (usize, bool) {
147+
if self.len == 0 {
148+
return (0, false);
157149
}
158150

159-
// Binary search to find insertion point
160151
let mut left = 0;
161152
let mut right = self.len as usize;
162153

@@ -166,12 +157,9 @@ where
166157
// Safety: mid is always < self.len due to binary search bounds
167158
let mid_key = unsafe { self.key_at(mid) };
168159

169-
match mid_key.cmp(&key) {
160+
match mid_key.cmp(key) {
170161
std::cmp::Ordering::Equal => {
171-
// Key exists - update value and return old value
172-
let old_value = unsafe { *self.value_at(mid) };
173-
unsafe { self.set_value_at(mid, value) };
174-
return Ok(Some(old_value));
162+
return (mid, true); // Found exact match
175163
}
176164
std::cmp::Ordering::Less => {
177165
left = mid + 1;
@@ -182,8 +170,35 @@ where
182170
}
183171
}
184172

185-
// Key doesn't exist - insert at position 'left'
186-
let insert_pos = left;
173+
(left, false) // Not found, return insertion point
174+
}
175+
}
176+
177+
// Placeholder implementations - will be implemented through TDD
178+
impl<K, V> CompressedLeafNode<K, V>
179+
where
180+
K: Copy + Ord,
181+
V: Copy,
182+
{
183+
/// Insert a key-value pair into the leaf.
184+
/// Returns Ok(Some(old_value)) if key existed, Ok(None) if new key, Err if full.
185+
pub fn insert(&mut self, key: K, value: V) -> Result<Option<V>, &'static str> {
186+
let (index, found) = self.find_key_index(&key);
187+
188+
if found {
189+
// Key exists - update value and return old value
190+
let old_value = unsafe { *self.value_at(index) };
191+
unsafe { self.set_value_at(index, value) };
192+
return Ok(Some(old_value));
193+
}
194+
195+
// Key doesn't exist - check capacity
196+
if self.len >= self.capacity {
197+
return Err("Leaf is at capacity");
198+
}
199+
200+
// Insert new key at the found position
201+
let insert_pos = index;
187202
let current_len = self.len as usize;
188203

189204
// Shift keys and values to make room
@@ -215,35 +230,13 @@ where
215230

216231
/// Get a value by key.
217232
pub fn get(&self, key: &K) -> Option<&V> {
218-
if self.len == 0 {
219-
return None;
220-
}
221-
222-
// Binary search through the keys
223-
let mut left = 0;
224-
let mut right = self.len as usize;
225-
226-
while left < right {
227-
let mid = left + (right - left) / 2;
228-
229-
// Safety: mid is always < self.len due to binary search bounds
230-
let mid_key = unsafe { self.key_at(mid) };
231-
232-
match mid_key.cmp(key) {
233-
std::cmp::Ordering::Equal => {
234-
// Found the key, return corresponding value
235-
return Some(unsafe { self.value_at(mid) });
236-
}
237-
std::cmp::Ordering::Less => {
238-
left = mid + 1;
239-
}
240-
std::cmp::Ordering::Greater => {
241-
right = mid;
242-
}
243-
}
233+
let (index, found) = self.find_key_index(key);
234+
235+
if found {
236+
Some(unsafe { self.value_at(index) })
237+
} else {
238+
None
244239
}
245-
246-
None // Key not found
247240
}
248241

249242
/// Remove a key-value pair from the leaf.
@@ -553,6 +546,38 @@ mod tests {
553546
assert_eq!(leaf.get(&i32::MAX), Some(&2000));
554547
}
555548

549+
#[test]
550+
fn test_find_key_index_helper() {
551+
let mut leaf = CompressedLeafNode::<i32, i32>::new(10);
552+
553+
// Test empty leaf
554+
assert_eq!(leaf.find_key_index(&42), (0, false));
555+
556+
// Manually set up data: keys [10, 20, 30]
557+
leaf.len = 3;
558+
unsafe {
559+
leaf.set_key_at(0, 10);
560+
leaf.set_key_at(1, 20);
561+
leaf.set_key_at(2, 30);
562+
}
563+
564+
// Test exact matches
565+
assert_eq!(leaf.find_key_index(&10), (0, true));
566+
assert_eq!(leaf.find_key_index(&20), (1, true));
567+
assert_eq!(leaf.find_key_index(&30), (2, true));
568+
569+
// Test insertion points for missing keys
570+
assert_eq!(leaf.find_key_index(&5), (0, false)); // Before first
571+
assert_eq!(leaf.find_key_index(&15), (1, false)); // Between 10 and 20
572+
assert_eq!(leaf.find_key_index(&25), (2, false)); // Between 20 and 30
573+
assert_eq!(leaf.find_key_index(&35), (3, false)); // After last
574+
575+
// Test boundary cases
576+
assert_eq!(leaf.find_key_index(&9), (0, false));
577+
assert_eq!(leaf.find_key_index(&11), (1, false));
578+
assert_eq!(leaf.find_key_index(&31), (3, false));
579+
}
580+
556581
// Phase 6: Remove Tests (will fail until implemented)
557582

558583
#[test]

0 commit comments

Comments
 (0)