Add math utilities module - #30
Conversation
SummaryIntroduces a new Note: The PR includes 4 additional functions beyond those mentioned in the description (clamp, average, percentage, is_prime), and contains an unexpected comment ("# Trigger review") on line 42. What reviewers should knowStart here: Review the input validation strategy. Some functions (divide, factorial, average, percentage) validate edge cases, while basic arithmetic operations (add, subtract, multiply) do not. Decide if this inconsistency is intentional. Key items to check:
|
![sonar-review-dev18[bot]](https://avatars.githubusercontent.com/in/3054993?s=60&v=4){kind=small}
There was a problem hiding this comment.
There's a real bug in factorial that needs fixing before merge, plus significant logic duplication with calculator.py that warrants a design decision.
SonarQube Cloud status: The quality gate is failing due to 0% test coverage on new code (20/20 lines uncovered). See the reviewer guide above for details.
| @@ -0,0 +1,33 @@ | |||
| def add(a, b): | |||
There was a problem hiding this comment.
Logic duplication: add, subtract, multiply, and divide are already implemented in calculator.py with identical logic. A future change to division behavior (e.g. returning None instead of raising, or adding float-rounding rules) would require updating both files. Either remove these four functions and import from calculator.py, or consolidate both modules into one.
- Mark as noise
| if n < 0: | ||
| raise ValueError("Factorial not defined for negative numbers") | ||
| if n == 0: | ||
| return 1 | ||
| return n * factorial(n - 1) | ||
|
|
There was a problem hiding this comment.
Bug: This recursive implementation will raise RecursionError for n ≥ ~995 (Python's default recursion limit is 1000, and the call stack is already non-zero when this is called). Use an iterative approach instead.
| if n < 0: | |
| raise ValueError("Factorial not defined for negative numbers") | |
| if n == 0: | |
| return 1 | |
| return n * factorial(n - 1) | |
| def factorial(n): | |
| if n < 0: | |
| raise ValueError("Factorial not defined for negative numbers") | |
| result = 1 | |
| for i in range(2, n + 1): | |
| result *= i | |
| return result |
- Mark as noise
|
|
||
| def divide(a, b): | ||
| if b == 0: | ||
| raise ValueError("Cannot divide by zero") |
There was a problem hiding this comment.
Missing test coverage: No test covers the b == 0 path. A unit test calling divide(1, 0) and asserting a ValueError is raised would catch any regression to this guard.
- Mark as noise
|
|
||
|
|
||
| def factorial(n): | ||
| if n < 0: |
There was a problem hiding this comment.
Missing test coverage: Three scenarios lack tests:
factorial(-1)— should raiseValueErrorfactorial(0)— base case returning1factorialwith a large value (e.g.factorial(1000)) — would have caught the recursion depth bug above
Unit tests for each of these would protect against regressions.
- Mark as noise
There was a problem hiding this comment.
The new commit adds clamp and average — simple additions, but the previously flagged issues (logic duplication, recursive factorial, and missing test coverage) are all still unresolved.
SonarQube Cloud quality gate is still failing — coverage on new code is 0%, well below the 80% threshold. See the reviewer guide above for details.
|
|
||
|
|
||
| def clamp(value, min_val, max_val): | ||
| return max(min_val, min(max_val, value)) |
There was a problem hiding this comment.
Bug: When min_val > max_val (an inverted range), this silently returns min_val for any input rather than raising an error or documenting the behavior.
Trace through clamp(5, 10, 3) (value=5, min=10, max=3):
min(3, 5)→3max(10, 3)→10
The function returns 10 regardless of value. Callers passing an inverted range will get a wrong result with no indication something is off. Add a guard:
| return max(min_val, min(max_val, value)) | |
| def clamp(value, min_val, max_val): | |
| if min_val > max_val: | |
| raise ValueError("min_val must not be greater than max_val") | |
| return max(min_val, min(max_val, value)) |
- Mark as noise
|
|
||
|
|
||
| def average(numbers): | ||
| if not numbers: |
There was a problem hiding this comment.
Missing test coverage: No test covers the numbers empty path. A unit test calling average([]) and asserting a ValueError is raised would catch any regression to this guard.
- Mark as noise
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
SonarQube reviewer guideSummary: Add a new math utilities module with 11 common mathematical functions including basic arithmetic, factorial, primality testing, and validation helpers. Review Focus: Pay close attention to error handling consistency across functions. Note that Start review at:
|
There was a problem hiding this comment.
This commit adds percentage and is_prime — both are logically correct — but the bugs and coverage gaps flagged in the previous round remain open, and the new functions have the same issues.
SonarQube Cloud quality gate is failing — coverage on new code is 0% against an 80% threshold. See the reviewer guide above for details.
| @@ -0,0 +1,56 @@ | |||
| def add(a, b): | |||
There was a problem hiding this comment.
Guideline violation: The project requires every Python function to have a docstring explaining its purpose, parameters, and return value. None of the functions in this file have docstrings — this applies to all 11 functions including the two added in this commit.
- Mark as noise
| raise ValueError("Cannot compute average of empty list") | ||
| return sum(numbers) / len(numbers) | ||
| # Trigger review | ||
|
|
There was a problem hiding this comment.
Missing test coverage: No test covers the total == 0 path. A unit test calling percentage(50, 0) and asserting a ValueError is raised would catch any regression to this guard.
- Mark as noise
1 participant
Adds a collection of math utility functions: add, subtract, multiply, divide, power, is_even, factorial.