-
Notifications
You must be signed in to change notification settings - Fork 1
add utility functions #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,39 @@ | ||||||
| def calculate_discount(price, percentage): | ||||||
| return price - (price * percentage / 100) | ||||||
|
|
||||||
|
|
||||||
| def is_valid_email(email): | ||||||
| return "@" in email and "." in email.split("@")[-1] | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: A minimal fix is to also assert there is exactly one
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage: The email validation logic is non-trivial and entirely uncovered. At minimum, tests should assert:
|
||||||
|
|
||||||
|
|
||||||
| def format_user_name(first, last): | ||||||
| return f"{first.strip().capitalize()} {last.strip().capitalize()}" | ||||||
|
|
||||||
|
|
||||||
| def clamp(value, min_value, max_value): | ||||||
| if value < min_value: | ||||||
| return min_value | ||||||
| if value > max_value: | ||||||
| return max_value | ||||||
| return value | ||||||
|
|
||||||
|
|
||||||
| def truncate_string(text, max_length): | ||||||
| if len(text) <= max_length: | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: When truncation occurs, the returned string is For example: Callers that rely on the result fitting within
Suggested change
|
||||||
| return text | ||||||
| return text[:max_length] + "..." | ||||||
|
|
||||||
|
|
||||||
| def parse_int_safe(value): | ||||||
| try: | ||||||
| return int(value) | ||||||
| except (ValueError, TypeError): | ||||||
| return None | ||||||
|
|
||||||
|
|
||||||
| def slugify(text): | ||||||
| return text.lower().strip().replace(" ", "-") | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug:
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('-')
|
||||||
|
|
||||||
|
|
||||||
| def pluralize(word, count): | ||||||
| return word if count == 1 else word + "s" | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage:
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 clampingpercentageto[0, 100], would prevent this from reaching production.