Skip to content

Format zero-decimal currencies like JPY without decimals - #31

Open
sergical wants to merge 1 commit into
mainfrom
feat/currency-zero-decimal
Open

sergical wants to merge 1 commit into
mainfrom
feat/currency-zero-decimal

Conversation

@sergical

Copy link
Copy Markdown
Member

Adds formatAmount, which formats an amount in minor units for any currency. JPY, KRW and VND have no minor unit, so they print as whole numbers; other currencies get two decimals.

Created with Claude Code

@github-actions

Copy link
Copy Markdown

Verdict: Approve with minor suggested changes (no blocking issues, but a few correctness edge cases worth addressing before merge).

Correctness

  • No automated tests exist for this file/function (run_tests found none), so these edge cases are currently unverified.
  • ZERO_DECIMAL_CURRENCIES.has(currency) is case-sensitive. A lowercase or mixed-case code (e.g. "jpy") silently falls through to the /100 path, producing a wrong result like "12.00 jpy" instead of "1200 jpy". Normalize with currency.toUpperCase() before the check and in the output.
  • No validation of minorUnits for the zero-decimal branch — a fractional value would print as-is (e.g. "100.5 JPY"), which isn't a sensible zero-decimal amount. Similarly, NaN/Infinity inputs produce "NaN USD" / "Infinity USD" with no guard.
  • Negative amounts aren't considered; toFixed(2) on negatives works mechanically ("-12.00 USD") but whether that's the intended display format is untested.
  • The zero-decimal currency list is small and hardcoded (JPY, KRW, VND); ISO 4217 has more zero-decimal currencies (CLP, XOF, XAF, PYG, ISK, etc.). Fine if scope is intentionally limited, but worth a comment noting it's not exhaustive.
  • Pre-existing formatUsd doesn't use toFixed(2), so it's inconsistent with the new formatAmount (e.g. formatUsd(200) → "$2" vs formatAmount(200, "USD") → "2.00 USD"). Not a regression, but the new code highlights the inconsistency.

Style

  • No comment explaining why JPY/KRW/VND were chosen for ZERO_DECIMAL_CURRENCIES or that the list isn't exhaustive — would help future maintainers extend it correctly.
  • Output format diverges from formatUsd (symbol-prefixed "$2.00" vs. code-suffixed "2.00 USD"); consider a short note on why the two helpers use different conventions, or align them.
  • Missing JSDoc on the new exported formatAmount explaining minorUnits semantics (cents vs. whole units depending on currency) — easy for callers to misuse.
  • Since formatAmount(cents, "USD") now overlaps functionally with formatUsd, consider having formatUsd delegate to formatAmount to avoid duplicated/divergent logic.
  • Loose string type for currency offers no compile-time safety against invalid codes; a union/enum could help if the currency set is bounded.

Overall this is a small, well-contained addition; the main things worth fixing before merge are the case-sensitivity bug and adding minimal input validation/documentation.

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