Skip to content

add numberWithCommas util#6

Merged
rmartin93 merged 2 commits into
masterfrom
claude/issue-4-20260107-1443
Jan 7, 2026
Merged

add numberWithCommas util#6
rmartin93 merged 2 commits into
masterfrom
claude/issue-4-20260107-1443

Conversation

@rmartin93

Copy link
Copy Markdown
Collaborator

Adds TypeScript utility function for formatting numbers with comma separators

Closes #4

🤖 Generated with Claude Code

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-authored-by: Ryan Martin <rmartin93@users.noreply.github.com>
@claude

claude Bot commented Jan 7, 2026

Copy link
Copy Markdown

PR Review

Overall: Well-structured addition. Code follows repo conventions and implements a useful utility function.

✅ Strengths

  • Matches existing style (consistent w/ strings.ts)
  • Self-contained w/ no dependencies (per design principles)
  • Clear comments and function naming
  • Proper export structure via index.ts
  • Good handling of optional decimal places

🔍 Issues Found

Bug: Negative number handling (numbers.ts:15)
Current regex breaks negative numbers:

numberWithCommas(-1234.56, 2) // Returns "-,1234.56" ❌

Fix:

// Handle negative sign
const isNegative = integerPart.startsWith("-");
const absoluteInt = isNegative ? integerPart.slice(1) : integerPart;
const withCommas = absoluteInt.replace(/\B(?=(\d{3})+(?!\d))/g, ",");
return (isNegative ? "-" : "") + (decimalPart !== undefined ? `${withCommas}.${decimalPart}` : withCommas);

💡 Recommendations

Add edge case tests - Consider these scenarios:

  • Negative numbers: numberWithCommas(-1234.56, 2)
  • Zero: numberWithCommas(0, 2)
  • Large numbers: numberWithCommas(1234567890)
  • Small decimals: numberWithCommas(0.001, 3)

Add JSDoc example:

/**
 * Format number with comma separators and optional decimal places
 * @example numberWithCommas(1234.567, 2) // "1,234.57"
 */

📊 Assessment

  • Code Quality: 8/10 (would be 10/10 with negative fix)
  • Security: ✅ No concerns
  • Performance: ✅ Efficient regex approach
  • Test Coverage: ⚠️ None (acceptable for snippet library but recommend manual testing of edge cases)

Recommendation: Fix negative number handling before merge, then LGTM.

@rmartin93

Copy link
Copy Markdown
Collaborator Author

@claude can you fix the negative number issue?

@claude

claude Bot commented Jan 7, 2026

Copy link
Copy Markdown

Claude finished @rmartin93's task in 1m 1s —— View job


Negative number bug fixed ✓

  • Read current implementation
  • Fix negative number handling
  • Commit and push changes

Summary: Fixed negative number handling by extracting the negative sign before applying comma regex, then prepending it back to the result.

Examples of fixed behavior:

  • numberWithCommas(-1234.56, 2)"-1,234.56" ✓ (was "-,1234.56")
  • numberWithCommas(-1000000)"-1,000,000"
  • numberWithCommas(1234.56, 2)"1,234.56" ✓ (unchanged)

|

Co-authored-by: Ryan Martin <rmartin93@users.noreply.github.com>
@claude

claude Bot commented Jan 7, 2026

Copy link
Copy Markdown

PR Review: numberWithCommas utility

Strengths

  • Well-commented - matches existing code style in strings.ts
  • Self-contained - no dependencies, aligns with design principles
  • Proper export - correctly added to index.ts barrel export
  • Edge case handling - addresses negative numbers and decimal places

Potential Issues

1. Type coercion bug (ts/utilities/numbers.ts:16)

decimalPart !== undefined ? `${withCommas}.${decimalPart}` : withCommas

Issue: split() returns string[] | undefined for second element, but checking !== undefined. Empty string after decimal (e.g., "5.") would pass this check incorrectly.

Fix: Check decimalPart != null or restructure logic.

2. Inconsistent decimal handling

  • num.toFixed(decimalPlaces) always adds decimals when specified
  • But num.toString() preserves original format
  • Example: numberWithCommas(5.0) returns "5" but numberWithCommas(5.0, 2) returns "5.00"

This may be intentional, but consider documenting behavior.

Edge Cases to Consider

Test these scenarios:

  • numberWithCommas(0) = "0" (OK)
  • numberWithCommas(-0) = "-0" or "0"? (likely "-0")
  • numberWithCommas(1000.001, 2) = "1,000.00" (rounds, may surprise users)
  • numberWithCommas(NaN) = "NaN" (no commas, acceptable)
  • numberWithCommas(Infinity) = "Infinity" (no commas, acceptable)

Suggestions

Add usage examples in comments:

// Format number with comma separators and optional decimal places
// Examples:
//   numberWithCommas(1234567) = "1,234,567"
//   numberWithCommas(1234.5, 2) = "1,234.50"
//   numberWithCommas(-9876543.21) = "-9,876,543.21"
export function numberWithCommas(num: number, decimalPlaces?: number): string {

Consider renaming to match existing patterns:

  • Current: numberWithCommas
  • Alternative: formatNumber (mirrors strings.ts naming like capitalize, truncate)

Overall Assessment

Code Quality: 8/10 - Clean, readable, follows repo conventions
Correctness: 7/10 - One potential undefined check bug
Completeness: 9/10 - Handles most cases well

Recommendation: Approve with minor fix

Fix the decimalPart !== undefined check on line 16, then merge. Great addition to the utilities library.


Review automated via Claude Code

@rmartin93 rmartin93 merged commit b5446c0 into master Jan 7, 2026
1 check passed
@rmartin93 rmartin93 deleted the claude/issue-4-20260107-1443 branch January 7, 2026 14:51
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.

Number with Commas

1 participant