Skip to content

Keep discounts between 0 and 100 percent - #36

Open
sergical wants to merge 1 commit into
mainfrom
fix/discount-clamp-percent
Open

sergical wants to merge 1 commit into
mainfrom
fix/discount-clamp-percent

Conversation

@sergical

Copy link
Copy Markdown
Member

applyDiscount now clamps the percent to the range 0 to 1, so a bad discount code can no longer raise the total or make it negative.

Created with Claude Code

@github-actions

Copy link
Copy Markdown

Verdict: Approve with minor suggestions — the fix is a reasonable, low-risk improvement, but a couple of edge cases are worth a follow-up.

Correctness

  • No failing tests (none exist for this file — run_tests reports 0 ran).
  • NaN handling: if percent is NaN, Math.min(Math.max(NaN, 0), 1) evaluates to NaN, so the clamp doesn't actually guard against bad/missing input — rate and the final result both become NaN silently. Worth deciding whether to validate/throw instead of silently propagating.
  • Scale ambiguity: the clamp assumes percent is a 0–1 fraction. If any caller passes a 0–100 style percentage (e.g. 20 for 20%), this change silently clamps it to 1 (100% discount) rather than surfacing the unit mismatch — a behavioral change that could mask a caller bug. Worth confirming the contract for percent is documented/enforced elsewhere.
  • total is still unvalidated — a negative total flows through unchanged, though that's pre-existing behavior, not a regression introduced here.
  • These are edge cases rather than outright bugs in the stated intent (clamp percent to [0,1]); the core change does what it says for well-formed inputs.

Style

  • The intent of the clamp isn't obvious from the code alone — consider a short comment or a small clamp(value, min, max) helper for clarity and reuse.
  • rate is a slightly generic name relative to percent; clampedPercent/safePercent would make the relationship to the input clearer.
  • The magic numbers 0 and 1 could be named constants (e.g. MIN_PERCENT/MAX_PERCENT) to document why those bounds were chosen.

Overall this is a small, sensible defensive fix. Recommend considering explicit handling of NaN input and confirming the expected scale (0–1 vs 0–100) for percent before merging, but neither blocks approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant