Skip to content

Conversation

@Shane32
Copy link
Owner

@Shane32 Shane32 commented Oct 7, 2025

Summary by CodeRabbit

  • Performance
    • Faster PostScript QR code generation with reduced memory allocations and pre-sized output buffers.
  • Refactor
    • Normalized PostScript output assembly and inline newline formatting without altering rendered results.
  • Style
    • Unified numeric formatting for doubles (up to 7 significant digits, invariant culture).
  • Documentation
    • Updated comments to reflect new return semantics and formatting.
  • Tests
    • Refreshed approval snapshots to match the revised PostScript textual layout.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 7, 2025

📝 Walkthrough

Walkthrough

Refactors PostScript generation in QRCoder/PostscriptQRCode.cs to build output with a pre-sized StringBuilder and updates CleanSvgVal to format doubles with "G7" using invariant culture. Approved PostScript test baselines were updated to match the new textual output formatting and newline placement.

Changes

Cohort / File(s) Summary
PostScript generator refactor
QRCoder/PostscriptQRCode.cs
Replace string concatenation with a pre-sized StringBuilder for header/functions/body/footer assembly; return sb.ToString(); update CleanSvgVal to use "G7" with InvariantCulture; adjust XML/doc comments.
Approval test baselines (PostScript)
QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_colors.approved.txt, QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_eps.approved.txt, QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_simple.approved.txt, QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_size.approved.txt, QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_size_no_quiet_zones.approved.txt
Reflowed PostScript instruction sequences: replace previous -b/-nl patterns with reorganized + b/nl inline formatting and reordered equivalent drawing lines to match new string assembly output.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

enhancement, performance

Suggested reviewers

  • gfoidl
  • codebude

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title Check ✅ Passed The title “Add postscript optimizations” succinctly and accurately reflects the primary focus of the changeset, which is improving the performance and memory usage of the PostScript QR code generation. It is concise, specific to the main change, and avoids unnecessary detail or noise. Teammates scanning the history can immediately understand that the pull request introduces optimizations related to PostScript output.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ps_opt

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 317c587 and a427f08.

📒 Files selected for processing (6)
  • QRCoder/PostscriptQRCode.cs (3 hunks)
  • QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_colors.approved.txt (1 hunks)
  • QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_eps.approved.txt (1 hunks)
  • QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_simple.approved.txt (1 hunks)
  • QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_size.approved.txt (1 hunks)
  • QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_size_no_quiet_zones.approved.txt (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: Test .NET 6.0 Windows
  • GitHub Check: Test .NET Core 2.1
  • GitHub Check: Test .NET Framework 4.6.2
  • GitHub Check: Test .NET Core 3.1
  • GitHub Check: Test .NET 5.0 Windows
  • GitHub Check: additional-tests
🔇 Additional comments (7)
QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_eps.approved.txt (1)

53-85: LGTM! Baseline correctly reflects StringBuilder formatting changes.

The updated PostScript drawing commands now append "nl" inline at the end of each row (except the last), matching the StringBuilder-based assembly logic introduced in the production code. The rendering output remains functionally identical.

QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_simple.approved.txt (1)

53-85: LGTM! Baseline correctly reflects StringBuilder formatting changes.

The drawing command sequence has been reformatted with inline "nl" tokens, consistent with the StringBuilder-based assembly in the production code. No functional changes to the rendered output.

QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_size.approved.txt (1)

53-85: LGTM! Baseline correctly reflects StringBuilder formatting changes.

The reformatted PostScript body aligns with the StringBuilder-based output from the production code, with "nl" tokens appended inline. Rendering semantics are preserved.

QRCoder/PostscriptQRCode.cs (2)

113-134: LGTM! StringBuilder optimization is well-implemented.

The transition from string concatenation to StringBuilder with precomputed capacity improves performance and reduces allocations. The loop logic correctly appends "nl\n" at the beginning of each iteration (except the first), effectively placing "nl" at the end of each row except the last, matching the updated test baselines.


141-142: LGTM! Number formatting improved.

The "G7" format specifier limits output to 7 significant digits while preserving precision, and using InvariantCulture ensures consistent formatting across locales.

QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_size_no_quiet_zones.approved.txt (1)

53-77: LGTM! Baseline correctly reflects StringBuilder formatting changes.

The PostScript body has been reformatted with inline "nl" tokens, consistent with the StringBuilder-based assembly. The reduced row count reflects the drawQuietZones=false setting, with rendering output functionally unchanged.

QRCoderTests/PostscriptQRCodeRendererTests.can_render_postscript_qrcode_colors.approved.txt (1)

53-85: LGTM! Baseline correctly reflects StringBuilder formatting changes.

The drawing commands have been reformatted with inline "nl" tokens while preserving the custom color definitions (red foreground, blue background). Rendering output remains functionally identical.

@Shane32 Shane32 self-assigned this Oct 7, 2025
@Shane32 Shane32 requested a review from gfoidl October 7, 2025 21:26
@Shane32 Shane32 added the performance Performance related enhancements or benchmarks label Oct 7, 2025
@Shane32 Shane32 added this to the 1.7.0 milestone Oct 7, 2025
@Shane32 Shane32 changed the title Add postscript optimizations Add postscript renderer optimizations Oct 7, 2025
Copy link
Collaborator

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really lovely your clean and concise PRs.

@Shane32 Shane32 merged commit d40c005 into master Oct 7, 2025
8 checks passed
@Shane32 Shane32 deleted the ps_opt branch October 7, 2025 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Performance related enhancements or benchmarks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants