Skip to content

Conversation

@Shane32
Copy link
Owner

@Shane32 Shane32 commented Oct 13, 2025

Next step in supporting multi mode QR codes. It requires an additional allocation, but it's slightly faster presumably due to less copying of data between methods.

Before

Method Mean Error StdDev Gen0 Allocated
CreateQRCode 47.33 us 0.903 us 1.508 us 0.1831 4.07 KB
CreateQRCodeLong 1,200.37 us 10.287 us 9.623 us - 11.05 KB
CreateQRCodeLongest 6,762.80 us 38.097 us 35.636 us - 46.41 KB

After

Method Mean Error StdDev Gen0 Allocated
CreateQRCode 46.69 us 0.928 us 1.140 us 0.1831 4.12 KB
CreateQRCodeLong 1,184.45 us 19.989 us 18.698 us - 11.09 KB
CreateQRCodeLongest 6,716.87 us 21.365 us 18.940 us - 46.45 KB

Summary by CodeRabbit

  • New Features
    • Added support for chaining multiple QR data segments, enabling encoding of mixed content in a single QR code.
    • Improved handling of ECI (Extended Channel Interpretation) by preserving mode information across chained segments.
    • Ensured accurate bit-length calculation and writing across chained segments for more reliable QR generation.
    • Streamlined bit array creation to seamlessly serialize data from consecutive segments without user intervention.

@Shane32 Shane32 added this to the 1.7.1 milestone Oct 13, 2025
@Shane32 Shane32 self-assigned this Oct 13, 2025
@Shane32 Shane32 added the enhancement A new feature or feature request label Oct 13, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 13, 2025

📝 Walkthrough

Walkthrough

Converts the private nested DataSegment from a readonly struct to a private class with get-only properties, adds a Next reference for chaining, and updates GetBitLength, ToBitArray, and WriteTo to traverse chained segments and accumulate length and bit writes across the chain.

Changes

Cohort / File(s) Change Summary
Data segment refactor and chaining
QRCoder/QRCodeGenerator/DataSegment.cs
Change type from readonly struct to class; introduce get-only properties (EncodingMode, CharacterCount, Data, EciMode) and HasEciMode; add mutable Next for chaining; update GetBitLength to include Next; modify ToBitArray to use WriteTo; update WriteTo to recursively process Next and return final index.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant G as QRCodeGenerator
    participant DS as DataSegment (current)
    participant N as Next DataSegment (optional)
    participant BA as BitArray

    G->>DS: ToBitArray()
    DS->>BA: Create BitArray with total length (via GetBitLength incl. chain)
    DS->>DS: WriteTo(BA, startIndex)
    note over DS: Encode mode, length, data bits
    alt Has Next
        DS->>N: WriteTo(BA, updatedIndex)
        note over N: Encode its bits and recurse further if chained
    end
    DS-->>G: Return BitArray
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

refactoring

Suggested reviewers

  • gfoidl

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.
Title Check ✅ Passed The title concisely and accurately summarizes the primary change by indicating the conversion of DataSegment from a struct to a class and the addition of chained segment capability, which aligns directly with the implemented refactoring and new chaining support in the code.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ 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 segmented

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4cb1f83 and 4f31d52.

📒 Files selected for processing (1)
  • QRCoder/QRCodeGenerator/DataSegment.cs (5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/QRCodeGenerator/DataSegment.cs (1)
QRCoder/QRCodeGenerator.cs (3)
  • DataSegment (126-132)
  • EncodingMode (743-758)
  • GetCountIndicatorLength (886-946)
⏰ 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). (5)
  • GitHub Check: Test .NET 5.0 Windows
  • GitHub Check: Test .NET Core 3.1
  • GitHub Check: Test .NET Core 2.1
  • GitHub Check: Test .NET 6.0 Windows
  • GitHub Check: additional-tests
🔇 Additional comments (3)
QRCoder/QRCodeGenerator/DataSegment.cs (3)

8-28: LGTM! Well-designed struct-to-class refactor with chaining support.

The conversion from struct to class is appropriate here because:

  • Enables the self-referencing Next property for chaining segments
  • Reduces copying overhead (as noted in the PR's performance benchmarks)
  • The slight increase in allocation (4.12 KB vs 4.07 KB) is offset by performance gains

The Next property with a public setter allows flexible chain construction while keeping the other properties immutable through get-only accessors.


61-72: Chaining logic in GetBitLength is correct.

The recursive accumulation properly includes the bit length of all chained segments. The calculation for modeIndicatorLength at line 63 correctly accounts for both ECI mode (when present: 4 bits ECI indicator + 8 bits ECI value + 4 bits mode = 16 total) and regular mode (4 bits).

Note: The recursive implementation assumes chains will not be deeply nested. Given QR code capacity constraints, this is a reasonable assumption, but be aware that circular references (e.g., A.Next = B, B.Next = A) would cause a stack overflow. Since DataSegment is private, such misuse should be preventable at the call sites.


94-120: Chaining logic in WriteTo is correct.

The method properly writes the current segment's data and then recursively delegates to Next.WriteTo when a chained segment exists. The index is correctly advanced after each write operation, and the final index after all chained segments is returned.

Same note as for GetBitLength: the recursive implementation could overflow if chains contain cycles. This is acceptable given the private scope and expected usage patterns.


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.

@Shane32 Shane32 requested a review from gfoidl October 13, 2025 14:45
@Shane32 Shane32 merged commit e875e72 into master Oct 13, 2025
8 checks passed
@Shane32 Shane32 deleted the segmented branch October 13, 2025 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement A new feature or feature request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants