add utility functions - #29
Conversation
SummaryAdds a new Note: The PR description mentions only 4 functions but the implementation includes 8. All functions are simple, synchronous utilities with no dependencies. What reviewers should knowStart here: The entire changeset is in a single new file Points for reviewers to consider:
These utilities appear ready for straightforward use cases. Assess whether the simplistic implementations match your project's requirements or if more robust alternatives are needed.
|
![sonar-review-dev18[bot]](https://avatars.githubusercontent.com/in/3054993?s=60&v=4){kind=small}
|
|
||
|
|
||
| def is_valid_email(email): | ||
| return "@" in email and "." in email.split("@")[-1] |
There was a problem hiding this comment.
Bug: is_valid_email accepts addresses with multiple @ signs as valid. "a@b@c.com".split("@")[-1] yields "c.com", which passes the . check — so "attacker@target@domain.com" returns True.
A minimal fix is to also assert there is exactly one @:
| return "@" in email and "." in email.split("@")[-1] | |
| return email.count("@") == 1 and "." in email.split("@")[-1] |
- Mark as noise
| @@ -0,0 +1,18 @@ | |||
| def calculate_discount(price, percentage): | |||
| return price - (price * percentage / 100) | |||
There was a problem hiding this comment.
Missing test coverage: No test covers the case where percentage > 100, which causes the function to return a negative price. If callers can pass arbitrary percentages, this silently produces an invalid result (e.g. calculate_discount(50, 150) → -25). A unit test asserting the return value is non-negative, or a guard clamping percentage to [0, 100], would prevent this from reaching production.
- Mark as noise
|
|
||
|
|
||
| def is_valid_email(email): | ||
| return "@" in email and "." in email.split("@")[-1] |
There was a problem hiding this comment.
Missing test coverage: The email validation logic is non-trivial and entirely uncovered. At minimum, tests should assert:
-
Valid address returns
True -
Missing
@returnsFalse -
Multiple
@signs returnFalse -
Domain with no
.(e.g."user@localhost") returnsFalse -
Trailing dot in domain (e.g.
"user@domain.") returnsFalse -
Mark as noise
There was a problem hiding this comment.
There's a bug in the newly added truncate_string that needs fixing. The previously flagged issues (is_valid_email accepting multiple @ signs, calculate_discount producing negative prices, and missing email validation tests) are still open.
SonarQube Cloud status: Coverage on new code is 0% against an 80% threshold — see the reviewer guide above for details.
|
|
||
|
|
||
| def truncate_string(text, max_length): | ||
| if len(text) <= max_length: |
There was a problem hiding this comment.
Bug: When truncation occurs, the returned string is max_length + 3 characters long (the "..." suffix is appended after slicing to max_length), so the result silently exceeds the stated limit.
For example: truncate_string("hello world", 5) → "hello..." (8 chars, not 5).
Callers that rely on the result fitting within max_length (e.g. for a database column or UI label) will get data that's longer than expected.
| if len(text) <= max_length: | |
| return text[:max(0, max_length - 3)] + "..." |
- Mark as noise
There was a problem hiding this comment.
The four issues flagged in earlier review rounds (multiple-@ email bug, negative-price from calculate_discount, off-by-3 in truncate_string, and the email validation coverage gap) are all still present in the current code — none have been addressed.
SonarQube Cloud quality gate is failing — coverage on new code is 0% against an 80% threshold. See the reviewer guide above for details.
|
|
||
|
|
||
| def slugify(text): | ||
| return text.lower().strip().replace(" ", "-") |
There was a problem hiding this comment.
Bug: slugify only replaces spaces — it leaves punctuation, symbols, and non-ASCII characters in the output. For example:
slugify("Hello, World!")→"hello,-world!"(comma and!retained)slugify("café au lait")→"café-au-lait"(accented character retained)slugify("hello world")→"hello--world"(double space → double hyphen)
Any caller using the result in a URL will silently get an invalid slug. At minimum, non-word characters should be stripped and runs of hyphens collapsed:
import re
def slugify(text):
text = text.lower().strip()
text = re.sub(r'[^\w\s-]', '', text)
text = re.sub(r'[\s_]+', '-', text)
return re.sub(r'-+', '-', text).strip('-')- Mark as noise
SonarQube reviewer guide
|
There was a problem hiding this comment.
This commit only adds pluralize — the five issues flagged in prior review rounds (invalid email with multiple @ signs, negative price from calculate_discount, email validation coverage, truncate_string exceeding max_length, and slugify leaving punctuation/non-ASCII characters) are all still open and unaddressed.
SonarQube Cloud quality gate is failing — coverage on new code is 0%, against an 80% threshold. See the reviewer guide above for details.
|
|
||
|
|
||
| def pluralize(word, count): | ||
| return word if count == 1 else word + "s" |
There was a problem hiding this comment.
Missing test coverage: pluralize has no tests. Two cases that could regress silently:
-
count == 0(e.g.pluralize("item", 0)) should return the plural form — confirm the intent matches the call sites. -
Non-English words that don't pluralise with a plain
+"s"(e.g."bus"→"buss"instead of"buses") — if the project only uses simple English nouns this is fine, but it's worth a test to document the expected scope. -
Mark as noise
1 participant
Adds utility functions for discount calculation, email validation, name formatting, and value clamping.