Skip to content

Keep the zero padding on order ids - #30

Open
sergical wants to merge 1 commit into
mainfrom
fix/order-id-zero-pad
Open

sergical wants to merge 1 commit into
mainfrom
fix/order-id-zero-pad

Conversation

@sergical

Copy link
Copy Markdown
Member

nextOrderId now keeps the width of the previous id, so 00041 is followed by 00042 instead of 42. It also passes radix 10 to parseInt.

Created with Claude Code

@github-actions

Copy link
Copy Markdown

Verdict: Needs work — the padding fix regresses on digit-count overflow (e.g. "0099" → "100" instead of "0100"), and pre-existing edge cases (non-numeric input, NaN, negative numbers) remain unhandled.

Correctness

  • Overflow not handled: String(next).padStart(last.length, "0") only pads when the result is shorter than last.length. When incrementing causes the digit count to grow (e.g. "0099" → 100), padStart is a no-op, silently returning "100" instead of the expected fixed-width "0100". This is actually worse than a crash because it silently breaks the fixed-width invariant the padding was meant to guarantee — any downstream code assuming result.length === last.length will break or produce ID collisions.
  • Non-numeric / malformed input unhandled: If last has a non-numeric prefix/suffix (e.g. "ORD-0007") or is empty, parseInt either silently parses a partial value or returns NaN, producing "NaN" (possibly zero-padded) as the "next" order id with no error. This bug pre-existed the change but the PR is a good opportunity to guard against it since it's directly touching this logic.
  • Negative numbers: last = "-5" → next = -4 → String(-4).padStart(3, "0") yields "0-4", a malformed id with the sign in the wrong place. Not handled.
  • No tests were found/run for this file (run_tests reported 0 ran), so none of the above is currently covered by CI.

Style

  • next is a fine but slightly generic name; consider nextIdNumber or nextValue to make the intent clearer, since it's a numeric value distinct from the final padded id string.

Tests

  • run_tests found no related tests (0 ran). Recommend adding unit tests covering: normal increment, digit-count overflow ("0099" → "0100"/"100" decision needs to be made explicit), non-numeric input, and empty string.

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