slices: introduce SliceP()

[karasz]

User description

returning the p-th percentile of a slice

---

PR Type

Enhancement

---

Description

• Introduces SliceP() function for percentile calculations

• Implements doP() helper for generic percentile computation

• Adds comprehensive test coverage for integer, float, and string types

• Updates documentation with new utility function

---

Diagram Walkthrough

flowchart LR
  A["Input Slice + Percentile p"] --> B["doP: Sort & Calculate Index"]
  B --> C["Return Element at Percentile"]
  D["Test Cases: int, float64, string"] --> E["Validate Edge Cases"]
  E --> F["Empty, Invalid p, Duplicates"]

Loading

File Walkthrough

	Relevant files
Enhancement	slices.go Implement percentile calculation functions                              slices.go Adds SliceP[T Ordered]() public function to return p-th percentile element Implements doP[T any]() helper function with in-place sorting and index calculation Includes comprehensive documentation with usage examples Handles edge cases: empty slices, invalid percentiles (p < 0 or p > 1) +42/-0
Tests	slices_test.go Add comprehensive percentile function tests                            slices_test.go Adds slicePTestCase[T Ordered] test case structure with factory function Implements TestSliceP() with 20+ test cases covering integers, floats, and strings Tests edge cases: empty slices, single elements, min/max percentiles, invalid inputs Tests real-world scenarios: unsorted data, negative numbers, duplicates +94/-0
Documentation	README.md Document new SliceP utility function                                          README.md Adds SliceP[T](slice,p) to utilities documentation section Describes function as returning the p-th percentile of the slice +1/-0

---

Summary by CodeRabbit

• New Features

  • Added a percentile retrieval to obtain the p-th percentile from ordered slices (returns zero value for empty or invalid inputs).

• Documentation

  • Added user-facing documentation and examples for the new percentile operation under Slice Operations.

• Tests

  • Added comprehensive tests for ints, floats and strings covering empty inputs, single-element, min/max, common percentiles, duplicates and invalid p values.

[coderabbitai]

Walkthrough

Adds an exported generic function SliceP[T](a []T, p float64) and internal helper doP to compute the p-th percentile by sorting the slice in place; documents the function in README and adds comprehensive tests for int, float64, and string.

Changes

Cohort / File(s)	Summary
Documentation README.md	Added documentation for the new exported utility SliceP[T](slice, p) under Slice Operations describing percentile retrieval, input constraints (p in 0.0–1.0), and in-place sorting behaviour.
Implementation slices.go	Added func SliceP[T Ordered](a []T, p float64) T and internal doP[T any](x []T, p float64, less func(a, b T) bool) T; validates inputs, sorts slice in-place using provided comparator, computes a rounded percentile index, clamps to bounds, and returns the element (zero value for empty slice or invalid p).
Tests slices_test.go	Added test scaffolding and TestSliceP covering int, float64, and string with cases for empty, single-element, P0/P50/P95/P99/P100, unsorted inputs, negatives, duplicates and invalid p values.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant SliceP
  participant doP
  participant Sort

  Caller->>SliceP: SliceP(slice, p)
  SliceP->>doP: doP(slice, p, less)
  note right of doP `#DDEEFF`: validate inputs\n(empty or invalid p -> zero value)\ncompute index = round(p*(n-1))\nclamp index
  doP->>Sort: sort slice in-place using less
  Sort-->>doP: sorted slice
  doP-->>SliceP: return element at index
  SliceP-->>Caller: percentile element

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Review focus:
  • In-place sort mutation and caller expectations (slices.go).
  • Rounding strategy for percentile index and p bounds handling (doP).
  • Zero-value behaviour for empty slices across generic types.
  • Tests coverage and edge-case correctness (slices_test.go).

Suggested labels

tests

Poem

🐰 I hopped through ordered rows and counted each pace,
Rounded p, clamped the bounds, left no messy trace,
Sorted the carrots and chose one with care,
A percentile nibble, perfect and fair,
I twitched my nose and danced on a tiny space.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title 'slices: introduce SliceP()' directly and specifically describes the main change: the introduction of the SliceP function to the slices package.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch pr-karasz-percentile

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 80b4dc7 and 05ecaea.

📒 Files selected for processing (3)

• README.md (1 hunks)
• slices.go (1 hunks)
• slices_test.go (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

• README.md
• slices_test.go
• slices.go

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Analyze (go)

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[deepsource-io]

Here's the code health analysis summary for commits 9e3b9f0..05ecaea. View details on DeepSource ↗.

Analysis Summary

Analyzer	Status	Summary	Link
Shell	✅ Success		View Check ↗
Go	✅ Success		View Check ↗

---

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[qodo-code-review]

PR Compliance Guide 🔍

(Compliance updated until commit 05ecaea)

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No audit logs: The new percentile utilities perform no critical system actions and introduce no logging, but given limited diff context it is unclear whether any critical actions elsewhere require audit logging. Referred Code // SliceP returns the p-th percentile element from a slice. // The slice is sorted in place (modifying the original slice) before // calculating the percentile. p must be between 0.0 (minimum) and 1.0 // (maximum). Returns the zero value for empty slices or when p is outside // the valid range. // This is commonly used for performance metrics and latency measurements // to identify outliers while excluding the worst values. // // Example: // // responseTimes := []int{100, 150, 120, 450, 89, 2000, 110, 130, 140, 160} // p99 := SliceP(responseTimes, 0.99) // Returns the value at the 99th percentile // // Note: responseTimes is now sorted func SliceP[T Ordered](a []T, p float64) T { result := doP(a, p, func(d, e T) bool { return d < e }) return result } // doP returns the element at the percentile position p (0.0 to 1.0) ... (clipped 21 lines)

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

---

Previous compliance checks

Compliance check up to commit 102e4ce

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No audit logs: The new percentile functions perform computations without any logging, but given these are utility functions and not critical actions, it is unclear whether audit logging is applicable here. Referred Code func SliceP[T Ordered](a []T, p float64) T { result := doP(a, p, func(d, e T) bool { return d < e }) return result } // doP returns the element at the percentile position p (0.0 to 1.0) // from a sortable slice. The slice will be sorted in place if needed. // p should be between 0.0 and 1.0, where 0.0 represents the minimum // and 1.0 represents the maximum. func doP[T any](x []T, p float64, less func(a, b T) bool) T { n := len(x) if n == 0 { var zero T return zero } if p < 0 || p > 1 { var zero T return zero } ... (clipped 10 lines)

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Use a standard library for statistics Replace the custom percentile implementation with a call to a standard, dedicated statistics library like gonum/stat. This ensures the calculation adheres to a well-defined standard, improving correctness and predictability for users. Examples: slices.go [300-319] func doP[T any](x []T, p float64, less func(a, b T) bool) T { n := len(x) if n == 0 { var zero T return zero } if p < 0 || p > 1 { var zero T return zero } ... (clipped 10 lines) Solution Walkthrough: Before: // file: slices.go func doP[T any](x []T, p float64, less func(a, b T) bool) T { n := len(x) if n == 0 { var zero T return zero } // ... input validation ... doSliceSort(x, less) // Custom "nearest rank" percentile calculation idx := max(int(float64(n)*p+0.5)-1, 0) if idx >= n { idx = n - 1 } return x[idx] } After: // file: slices.go // (Conceptual change, requires adding a dependency like gonum/stat) import "gonum.org/v1/gonum/stat" func SliceP[T Ordered](a []T, p float64) T { if len(a) == 0 { // ... handle empty slice ... } // Convert generic slice to []float64 for the library floatSlice := make([]float64, len(a)) for i, v := range a { floatSlice[i] = float64(v) } // Delegate to a standard, well-tested library. // Note: This example assumes the library can return a value // that can be mapped back to type T. The actual implementation // might require returning float64 or finding the closest element. percentileValue := stat.Percentile(floatSlice, p*100, nil) // gonum uses p in [0, 100] return T(percentileValue) // Simplified for illustration } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that the custom percentile calculation is a specific, non-standard method, which could be misleading; recommending a dedicated statistics library is a significant architectural improvement for correctness and standard compliance.	Medium
General	✅ Consolidate test helpers to reduce duplication Suggestion Impact: The commit refactored tests to reduce duplication by introducing factory functions and centralized test case constructors, and reorganized TestSliceP to run generated test case sets—aligning with the suggestion’s intent to consolidate repetitive test helpers into a generic, reusable approach. code diff: +// Factory function for slicePIntTestCase +func newSlicePIntTestCase(name string, input []int, p float64, expected int) slicePIntTestCase { + return slicePIntTestCase{ + name: name, + input: input, + p: p, + expected: expected, + } +} + // Test cases for SliceP function with float64 type slicePFloatTestCase struct { name string @@ -653,6 +663,16 @@ AssertEqual(t, tc.expected, result, "SliceP(%v, %.2f)", tc.input, tc.p) } +// Factory function for slicePFloatTestCase +func newSlicePFloatTestCase(name string, input []float64, p float64, expected float64) slicePFloatTestCase { + return slicePFloatTestCase{ + name: name, + input: input, + p: p, + expected: expected, + } +} + // Test cases for SliceP function with string type slicePStringTestCase struct { name string @@ -675,59 +695,70 @@ AssertEqual(t, tc.expected, result, "SliceP(%v, %.2f)", tc.input, tc.p) } +// Factory function for slicePStringTestCase +func newSlicePStringTestCase(name string, input []string, p float64, expected string) slicePStringTestCase { + return slicePStringTestCase{ + name: name, + input: input, + p: p, + expected: expected, + } +} + +func slicePIntTestCases() []TestCase { + return []TestCase{ + // Empty slice + newSlicePIntTestCase("empty slice", S[int](), 0.5, 0), + + // Single element + newSlicePIntTestCase("single element p=0.0", S(42), 0.0, 42), + newSlicePIntTestCase("single element p=0.5", S(42), 0.5, 42), + newSlicePIntTestCase("single element p=1.0", S(42), 1.0, 42), + + // Common percentiles + newSlicePIntTestCase("P50 (median)", S(5, 2, 8, 1, 9), 0.50, 5), + newSlicePIntTestCase("P95", S(1, 2, 3, 4, 5, 6, 7, 8, 9, 10), 0.95, 10), + newSlicePIntTestCase("P99", S(1, 2, 3, 4, 5, 6, 7, 8, 9, 10), 0.99, 10), + + // Min and max + newSlicePIntTestCase("P0 (min)", S(9, 3, 7, 1, 5), 0.0, 1), + newSlicePIntTestCase("P100 (max)", S(9, 3, 7, 1, 5), 1.0, 9), + + // Unsorted data + newSlicePIntTestCase("unsorted P25", S(10, 5, 8, 3, 1, 9, 4, 7, 2, 6), 0.25, 3), + newSlicePIntTestCase("unsorted P75", S(10, 5, 8, 3, 1, 9, 4, 7, 2, 6), 0.75, 8), + + // Negative numbers + newSlicePIntTestCase("negative P50", S(-5, -2, -8, -1, -9), 0.5, -5), + newSlicePIntTestCase("mixed signs P50", S(-2, 5, -1, 3, 0), 0.5, 0), + + // Duplicates + newSlicePIntTestCase("duplicates P50", S(3, 1, 3, 1, 3, 1, 3), 0.5, 3), + + // Invalid percentiles + newSlicePIntTestCase("invalid p=-0.1", S(1, 2, 3, 4, 5), -0.1, 0), + newSlicePIntTestCase("invalid p=1.5", S(1, 2, 3, 4, 5), 1.5, 0), + } +} + +func slicePFloatTestCases() []TestCase { + return []TestCase{ + newSlicePFloatTestCase("float P50", S(3.14, 1.41, 2.71, 0.5, 4.0), 0.5, 2.71), + newSlicePFloatTestCase("float P90", S(1.1, 2.2, 3.3, 4.4, 5.5, 6.6, 7.7, 8.8, 9.9, 10.0), 0.9, 9.9), + newSlicePFloatTestCase("float empty", S[float64](), 0.5, 0.0), + } +} + +func slicePStringTestCases() []TestCase { + return []TestCase{ + newSlicePStringTestCase("string P50", S("cherry", "apple", "banana", "date"), 0.5, "banana"), + newSlicePStringTestCase("string P0", S("zebra", "alpha", "beta"), 0.0, "alpha"), + newSlicePStringTestCase("string P100", S("zebra", "alpha", "beta"), 1.0, "zebra"), + } +} + func TestSliceP(t *testing.T) { - // Test with integers - intTestCases := []slicePIntTestCase{ - // Empty slice - {name: "empty slice", input: S[int](), p: 0.5, expected: 0}, - - // Single element - {name: "single element p=0.0", input: S(42), p: 0.0, expected: 42}, - {name: "single element p=0.5", input: S(42), p: 0.5, expected: 42}, - {name: "single element p=1.0", input: S(42), p: 1.0, expected: 42}, - - // Common percentiles - {name: "P50 (median)", input: S(5, 2, 8, 1, 9), p: 0.50, expected: 5}, - {name: "P95", input: S(1, 2, 3, 4, 5, 6, 7, 8, 9, 10), p: 0.95, expected: 10}, - {name: "P99", input: S(1, 2, 3, 4, 5, 6, 7, 8, 9, 10), p: 0.99, expected: 10}, - - // Min and max - {name: "P0 (min)", input: S(9, 3, 7, 1, 5), p: 0.0, expected: 1}, - {name: "P100 (max)", input: S(9, 3, 7, 1, 5), p: 1.0, expected: 9}, - - // Unsorted data - {name: "unsorted P25", input: S(10, 5, 8, 3, 1, 9, 4, 7, 2, 6), p: 0.25, expected: 3}, - {name: "unsorted P75", input: S(10, 5, 8, 3, 1, 9, 4, 7, 2, 6), p: 0.75, expected: 8}, - - // Negative numbers - {name: "negative P50", input: S(-5, -2, -8, -1, -9), p: 0.5, expected: -5}, - {name: "mixed signs P50", input: S(-2, 5, -1, 3, 0), p: 0.5, expected: 0}, - - // Duplicates - {name: "duplicates P50", input: S(3, 1, 3, 1, 3, 1, 3), p: 0.5, expected: 3}, - - // Invalid percentiles - {name: "invalid p=-0.1", input: S(1, 2, 3, 4, 5), p: -0.1, expected: 0}, - {name: "invalid p=1.5", input: S(1, 2, 3, 4, 5), p: 1.5, expected: 0}, - } - - RunTestCases(t, intTestCases) - - // Test with float64 - floatTestCases := []slicePFloatTestCase{ - {name: "float P50", input: S(3.14, 1.41, 2.71, 0.5, 4.0), p: 0.5, expected: 2.71}, - {name: "float P90", input: S(1.1, 2.2, 3.3, 4.4, 5.5, 6.6, 7.7, 8.8, 9.9, 10.0), p: 0.9, expected: 9.9}, - {name: "float empty", input: S[float64](), p: 0.5, expected: 0.0}, - } - - RunTestCases(t, floatTestCases) - - // Test with strings - stringTestCases := []slicePStringTestCase{ - {name: "string P50", input: S("cherry", "apple", "banana", "date"), p: 0.5, expected: "banana"}, - {name: "string P0", input: S("zebra", "alpha", "beta"), p: 0.0, expected: "alpha"}, - {name: "string P100", input: S("zebra", "alpha", "beta"), p: 1.0, expected: "zebra"}, - } - - RunTestCases(t, stringTestCases) -} + RunTestCases(t, slicePIntTestCases()) + RunTestCases(t, slicePFloatTestCases()) + RunTestCases(t, slicePStringTestCases()) Refactor the separate test helper functions (testDoPEmpty, testDoPInvalidNegative, testDoPInvalidGreaterThanOne) into a single, generic helper to reduce code duplication. slices_test.go [723-745] -func testDoPEmpty(t *testing.T) { - t.Helper() - intLess := func(a, b int) bool { return a < b } - - result := doP(S[int](), 0.5, intLess) - AssertEqual(t, 0, result, "empty slice should return zero value") +func testDoPEdgeCases[T comparable](t *testing.T, name string, input []T, p float64, expected T, less func(a, b T) bool) { + t.Run(name, func(t *testing.T) { + t.Helper() + result := doP(input, p, less) + AssertEqual(t, expected, result, "doP with %s", name) + }) } -func testDoPInvalidNegative(t *testing.T) { - t.Helper() - intLess := func(a, b int) bool { return a < b } - - result := doP(S(1, 2, 3, 4, 5), -0.5, intLess) - AssertEqual(t, 0, result, "negative percentile should return zero value") -} - -func testDoPInvalidGreaterThanOne(t *testing.T) { - t.Helper() - intLess := func(a, b int) bool { return a < b } - - result := doP(S(1, 2, 3, 4, 5), 1.5, intLess) - AssertEqual(t, 0, result, "percentile > 1.0 should return zero value") -} - [Suggestion processed] Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies code duplication in the test helpers and proposes a valid refactoring to a generic, table-driven approach, which improves test maintainability.	Low
	Remove redundant index boundary check Remove the redundant if idx >= n check, as the preceding calculation for idx ensures it will never be out of bounds. slices.go [313-316] idx := max(int(float64(n)*p+0.5)-1, 0) -if idx >= n { - idx = n - 1 -} Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies redundant code. The logic proves that the if idx >= n check is unreachable, so removing it simplifies the function without changing behavior.	Low
Update

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

slices_test.go (1)

610-642: Consider optimising struct field alignment.

The test case structure is well-designed with proper generic typing and factory function. However, reordering fields could reduce memory usage from 72 to 48 bytes per instance.

Apply this diff to optimise field alignment:

 type doPTestCase[T comparable] struct {
-	name     string
 	input    []T
 	less     func(T, T) bool
+	name     string
 	p        float64
 	expected T
 }

Based on static analysis.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9e3b9f0 and 102e4ce.

📒 Files selected for processing (3)

• README.md (1 hunks)
• slices.go (1 hunks)
• slices_test.go (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

slices.go (1)

constraints.go (1)

• Ordered (39-41)

slices_test.go (2)

testing.go (5)

• TestCase (249-252)
• T (22-33)
• AssertEqual (278-287)
• S (240-245)
• RunTestCases (260-267)

slices.go (1)

• SliceCopy (199-206)

🪛 GitHub Check: make (1.23)

slices_test.go

[failure] 661-661:
The line is 122 characters long, which exceeds the maximum of 120 characters. (lll)

---

[failure] 611-611:
fieldalignment: struct with 72 pointer bytes could be 48 (govet)

🪛 GitHub Check: make (1.25)

slices_test.go

[failure] 661-661:
The line is 122 characters long, which exceeds the maximum of 120 characters. (lll)

---

[failure] 611-611:
fieldalignment: struct with 72 pointer bytes could be 48 (govet)

🪛 golangci-lint (2.5.0)

slices_test.go

[error] 661-661: File is not properly formatted

(gofmt)

---

[error] 611-611: fieldalignment: struct with 72 pointer bytes could be 48

(govet)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Analyze (go)

🔇 Additional comments (7)

slices_test.go (4)

21-21: LGTM!

The compile-time interface check correctly verifies that doPTestCase[int] implements the TestCase interface, ensuring type safety at build time.

---

644-693: Comprehensive test coverage with excellent edge case handling.

The test suite thoroughly covers percentile calculations across multiple types and scenarios, including boundary conditions, unsorted inputs, and various distributions. The inline comments documenting the index calculation formula are particularly helpful.

---

695-713: Excellent coverage of additional data types.

Testing with float64 and string types ensures the generic implementation works correctly across different ordered types. The test cases appropriately verify minimum, median, and maximum percentiles.

---

715-745: Thorough edge case validation.

The separate test functions for empty slices and invalid percentile values ensure robust error handling. The expectation that invalid inputs return zero values is clearly documented and verified.

README.md (1)

187-187: LGTM!

The documentation clearly describes the new SliceP function and follows the established pattern for documenting slice operations.

slices.go (2)

279-294: Well-documented public API with clear usage example.

The function signature is clean, documentation explains the in-place sorting behaviour, and the example demonstrates practical usage for performance metrics. The implementation correctly delegates to the internal doP helper.

---

296-319: Solid percentile calculation with proper bounds handling.

The implementation correctly handles edge cases (empty slices, invalid percentiles) and uses the nearest-rank percentile method with appropriate rounding. The in-place sorting optimises memory usage whilst the bounds clamping ensures index safety.

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid modifying the input slice Modify SliceP to operate on a copy of the input slice, preventing the surprising side effect of sorting the original slice in-place. slices.go [279-294] // SliceP returns the p-th percentile element from a slice. -// The slice will be sorted in place before calculating the percentile. // This is commonly used for performance metrics and latency measurements // to identify outliers while excluding the worst values. // If the slice is empty, returns the zero value of type T. +// Note: This function works on a copy of the slice and does not modify the original. // // Example: // // responseTimes := []int{100, 150, 120, 450, 89, 2000, 110, 130, 140, 160} // p := SliceP(responseTimes,0.99) // Returns value at 99th percentile func SliceP[T Ordered](a []T, p float64) T { - result := doP(a, p, func(d, e T) bool { + if len(a) == 0 { + var zero T + return zero + } + // Work on a copy to avoid modifying the original slice + c := SliceCopy(a) + result := doP(c, p, func(d, e T) bool { return d < e }) return result } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the function modifies its input slice, which can be a surprising side effect. Proposing to work on a copy improves API safety and predictability, even though the current behavior is documented.	Low
More

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

slices_test.go (1)

626-634: Consider clarifying the comment.

The copy is created to prevent mutation of test data (since SliceP sorts in-place), not specifically for testing unsorted data. The current implementation is correct.

Consider this minor rewording:

-	// Create a copy to test with unsorted data
+	// Create a copy since SliceP sorts in-place
 	inputCopy := SliceCopy(tc.input)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 102e4ce and f2e635b.

📒 Files selected for processing (3)

• README.md (1 hunks)
• slices.go (1 hunks)
• slices_test.go (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• README.md
• slices.go

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-07-18T16:39:19.390Z

Learnt from: CR
Repo: darvaza-proxy/core PR: 0
File: AGENT.md:0-0
Timestamp: 2025-07-18T16:39:19.390Z
Learning: Applies to **/*.go : Max function length: 40 lines.

Applied to files:

• slices_test.go

🧬 Code graph analysis (1)

slices_test.go (3)

testing.go (5)

• TestCase (249-252)
• T (22-33)
• AssertEqual (278-287)
• S (240-245)
• RunTestCases (260-267)

constraints.go (1)

• Ordered (39-41)

slices.go (2)

• SliceCopy (199-206)
• SliceP (289-294)

🔇 Additional comments (4)

slices_test.go (4)

21-21: LGTM!

The compile-time interface verification follows the established pattern in the file.

---

610-620: LGTM!

The test case type is well-structured with appropriate generic constraints. The nolint directive for field alignment is reasonable given the generic type parameter.

---

636-644: LGTM!

The factory function follows the established pattern and provides type-safe test case construction.

---

646-701: Excellent test coverage!

The test suite comprehensively covers edge cases across multiple types (int, float64, string), including:

• Empty slices and single elements
• Various percentiles (P0, P25, P50, P75, P90, P95, P99, P100)
• Unsorted data, negative numbers, and duplicates
• Invalid percentile values (p < 0 or p > 1)

All test cases are mathematically correct and follow the established patterns in the codebase.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

slices_test.go (1)

646-701: Please keep TestSliceP within the 40-line cap

This project limits Go functions to 40 lines, yet TestSliceP currently runs ~56 lines. Please split the integer/float/string suites into helper functions or subtests so the top-level stays within the agreed ceiling. Based on learnings

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f2e635b and 8b3b0fe.

📒 Files selected for processing (3)

• README.md (1 hunks)
• slices.go (1 hunks)
• slices_test.go (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• slices.go

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-07-18T16:39:19.390Z

Learnt from: CR
Repo: darvaza-proxy/core PR: 0
File: AGENT.md:0-0
Timestamp: 2025-07-18T16:39:19.390Z
Learning: Applies to **/*.go : Max function length: 40 lines.

Applied to files:

• slices_test.go

🧬 Code graph analysis (1)

slices_test.go (3)

testing.go (5)

• TestCase (249-252)
• T (22-33)
• AssertEqual (278-287)
• S (240-245)
• RunTestCases (260-267)

constraints.go (1)

• Ordered (39-41)

slices.go (2)

• SliceCopy (199-206)
• SliceP (289-294)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Analyze (go)

[amery]

not a fan for disabling govet in this test file, but ok. TestSliceP() should be made of separate t.Run("bar", runFooBar) instead of a monster TestFoo

[karasz]

You are right, I will rewrite the tests to not be forced to disable govet.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

README.md (1)

187-187: Consider enhancing the documentation for SliceP.

The documentation is concise but could be more informative. Consider adding details about the valid range for p (0.0 to 1.0), the in-place sorting behaviour, and what happens with invalid inputs.

For example:

-* `SliceP[T](slice,p)` - return the p-th percentile of the slice
+* `SliceP[T](slice,p)` - return the p-th percentile of the slice (p must be 0.0-1.0). Sorts the slice in-place. Returns zero value for empty slices or invalid p.

slices.go (1)

279-294: Consider clarifying the in-place modification behaviour and parameter constraints.

The implementation is correct, but the documentation could be clearer about two important aspects:

1. The slice is modified in place, which might surprise users
2. The valid range for p (0.0 to 1.0) is mentioned in the internal doP function but should be explicit here

Consider revising the documentation:

 // SliceP returns the p-th percentile element from a slice.
-// The slice will be sorted in place before calculating the percentile.
+// The slice is sorted in place (modifying the original slice) before calculating the percentile.
+// p must be between 0.0 (minimum) and 1.0 (maximum). Returns the zero value for
+// empty slices or when p is outside the valid range.
 // This is commonly used for performance metrics and latency measurements
 // to identify outliers while excluding the worst values.
-// If the slice is empty, returns the zero value of type T.
 //
 // Example:
 //
 //	responseTimes := []int{100, 150, 120, 450, 89, 2000, 110, 130, 140, 160}
-//	p := SliceP(responseTimes,0.99) // Returns value at 99th percentile
+//	p99 := SliceP(responseTimes, 0.99) // Returns value at 99th percentile
+//	// Note: responseTimes is now sorted

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8b3b0fe and 38a1a6c.

📒 Files selected for processing (3)

• README.md (1 hunks)
• slices.go (1 hunks)
• slices_test.go (2 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-07-18T16:39:19.390Z

Learnt from: CR
Repo: darvaza-proxy/core PR: 0
File: AGENT.md:0-0
Timestamp: 2025-07-18T16:39:19.390Z
Learning: Applies to **/*.go : Max function length: 40 lines.

Applied to files:

• slices_test.go

🧬 Code graph analysis (2)

slices_test.go (2)

testing.go (5)

• TestCase (249-252)
• T (22-33)
• AssertEqual (278-287)
• S (240-245)
• RunTestCases (260-267)

slices.go (2)

• SliceCopy (199-206)
• SliceP (289-294)

slices.go (1)

constraints.go (1)

• Ordered (39-41)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Analyze (go)

🔇 Additional comments (2)

slices.go (1)

296-316: Implementation logic is correct.

The percentile calculation using the nearest-rank method with rounding is mathematically sound. The edge case handling (empty slices, invalid p values, boundary clamping with max()) is appropriate.

slices_test.go (1)

612-733: Excellent test coverage.

The test cases are comprehensive and well-structured:

• Properly implements the TestCase interface pattern
• Covers all critical scenarios: empty slices, single elements, common percentiles, edge cases (P0/P100), unsorted data, negative numbers, duplicates, and invalid inputs
• Tests across multiple types (int, float64, string)
• Uses SliceCopy to avoid test pollution
• Test expectations are mathematically correct

[amery]

@coderabbitai @karasz Updated to address the review feedback:

Documentation improvements:

• Added valid range for p parameter (0.0-1.0)
• Clarified in-place sorting behaviour with explicit warning
• Documented return values for edge cases

Test structure (TESTING.md compliant):

• Added factory functions for all three test case types
• Added test case list factories returning []TestCase
• Reduced TestSliceP from 56 lines to 5 lines

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@amery Thank you for the comprehensive updates! The improvements look great:

✅ Documentation - Adding the valid range (0.0-1.0), the in-place sorting warning, and edge case documentation will make the API much clearer for users.

✅ Test structure - The refactoring with factory functions and reducing TestSliceP from 56 to 5 lines demonstrates excellent adherence to the TESTING.md guidelines. This makes the tests much more maintainable and consistent with the project's testing patterns.

These changes address the review feedback effectively. Nice work on keeping the codebase clean and well-documented! 🎉

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

slices.go (2)

279-297: Document the percentile calculation method used.

The documentation clearly explains the function's purpose and behaviour, but it doesn't specify which percentile calculation method is used (e.g., nearest rank, linear interpolation, etc.). Different statistical methods can produce different results, particularly with small datasets. Adding this detail would help users understand the exact behaviour and verify results against other implementations.

Consider adding a sentence to the documentation such as:

 // SliceP returns the p-th percentile element from a slice.
 // The slice is sorted in place (modifying the original slice) before
 // calculating the percentile. p must be between 0.0 (minimum) and 1.0
-// (maximum). Returns the zero value for empty slices or when p is outside
-// the valid range.
+// (maximum). This implementation uses the nearest-rank method with rounding.
+// Returns the zero value for empty slices or when p is outside the valid range.

---

316-316: Add an inline comment explaining the percentile index calculation.

The formula int(float64(n)*p+0.5)-1 implements a nearest-rank percentile calculation with rounding, but this isn't immediately obvious. Adding a brief inline comment would improve code maintainability and help future readers understand the approach.

Consider adding a comment:

  doSliceSort(x, less)
  
+ // Calculate index using nearest-rank method: rank = round(n*p), then convert to 0-based index
  idx := max(int(float64(n)*p+0.5)-1, 0)
  
  return x[idx]

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 38a1a6c and 80b4dc7.

📒 Files selected for processing (3)

• README.md (1 hunks)
• slices.go (1 hunks)
• slices_test.go (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• README.md
• slices_test.go

🧰 Additional context used

🧬 Code graph analysis (1)

slices.go (1)

constraints.go (1)

• Ordered (39-41)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Analyze (go)

🔇 Additional comments (1)

slices.go (1)

309-312: Consider the implications of returning zero value for invalid percentile.

The function silently returns the zero value when p < 0 or p > 1. Whilst this defensive approach avoids panics, it means callers cannot distinguish between "invalid input" and "percentile of empty slice". For a percentile value that represents a programmer error (invalid p), this might mask bugs.

Consider whether this is the intended design, or if it would be better to panic for invalid p values (programmer error) whilst keeping the zero-value return only for empty slices (valid edge case). This aligns with Go conventions where invalid inputs to pure functions often panic (e.g., strings.Repeat with negative count).

[amery]

Released in v0.18.5.

FYI @karasz