Refactor DataSegment handling for improved encoding support #677
Conversation
…ses based on encoding mode
…nd simplify segment initialization
📝 WalkthroughWalkthroughReplaces inline text-to-bits logic with an abstract per-segment design: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant QRGen as QRCodeGenerator
participant Seg as DataSegment
participant Bits as BitArray
Caller->>QRGen: Generate(text, mode, options)
QRGen->>Seg: CreateSegment(text, mode, options)
Seg-->>QRGen: segment instance (Numeric|Alphanumeric|Byte)
QRGen->>Seg: GetBitLength(version)
Seg-->>QRGen: bitLength
QRGen->>Bits: Allocate(bitLength)
QRGen->>Seg: WriteTo(Bits, startIndex, version)
Seg-->>QRGen: nextIndex
Note right of Seg: ByteDataSegment may emit ECI header then data (optional UTF‑8 BOM)
QRGen-->>Caller: BitArray (encoded payload)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
QRCoder/QRCodeGenerator.cs(1 hunks)QRCoder/QRCodeGenerator/DataSegment.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
QRCoder/QRCodeGenerator.cs (1)
QRCoder/QRCodeGenerator/DataSegment.cs (6)
NumericDataSegment(63-119)NumericDataSegment(74-77)AlphanumericDataSegment(124-180)AlphanumericDataSegment(135-138)ByteDataSegment(185-275)ByteDataSegment(219-225)
QRCoder/QRCodeGenerator/DataSegment.cs (2)
QRCoder/QRCodeGenerator.cs (8)
DataSegment(126-138)EncodingMode(749-764)BitArray(996-1005)BitArray(1015-1056)BitArray(1079-1155)GetCountIndicatorLength(892-952)DecToBin(874-884)GetDataLength(962-968)QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (1)
AlphanumericEncoder(8-76)
⏰ 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 Core 3.1
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET Core 2.1
- GitHub Check: additional-tests
🔇 Additional comments (1)
QRCoder/QRCodeGenerator.cs (1)
129-137: LGTM! Clean refactoring with appropriate defensive programming.The type-dispatched factory pattern improves code structure by leveraging polymorphism. The switch expression correctly maps each encoding mode to its specialized segment class, and parameter passing aligns with the constructors defined in DataSegment.cs.
The default case provides good defensive programming, even though
GetEncodingFromPlaintext(lines 749-764) should only returnNumeric,Alphanumeric, orByteunder normal operation. The exception message is descriptive and aids debugging.
…or improved bit calculation and writing
Shane32
added
the
refactoring
|
This will clean up further once I have the Micro QR code code use the DataSegment class. But already this PR is "too large". |
|
I'm going to rewrite #676 to be a derived class of DataSegment so it can calculate the size of the text live with various versions (sizes) of QR code, and then only create the data when requested, and all at once to a single bitarray, instead of doing it a bit at a time and then combining all the bit arrays together. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (1)
67-98: Optional: add basic input/capacity validation to harden the writer.To prevent hard-to-debug IndexOutOfRange/KeyNotFound, consider early checks.
public static int WriteToBitArray(string plainText, BitArray codeText, int codeIndex) { + if (codeText == null) throw new ArgumentNullException(nameof(codeText)); + if (codeIndex < 0) throw new ArgumentOutOfRangeException(nameof(codeIndex)); + // Ensure target has enough room for the bits we are about to write + int required = GetBitLength(plainText); + if (codeText.Length - codeIndex < required) + throw new ArgumentException("Target BitArray is too small for the encoded data.", nameof(codeText)); + var index = 0; var count = plainText.Length; ... }QRCoder/QRCodeGenerator.cs (1)
1031-1045: CopyToBitArray bit ordering implementation looks correct.Writes bit j from MSB first using
(1 << (7 - j)). Consider guardingoffset >= 0.- private static void CopyToBitArray( + private static void CopyToBitArray( ... BitArray bitArray, int offset) { + if (offset < 0) throw new ArgumentOutOfRangeException(nameof(offset)); for (var i = 0; i < byteArray.Length; i++) {QRCoder/QRCodeGenerator/NumericDataSegment.cs (1)
57-61: Avoid intermediate BitArray allocation in WriteTo.Stream numeric bits directly into the target to reduce allocations.
- // write data - encode numeric text - var data = PlainTextToBinaryNumeric(Text); - data.CopyTo(bitArray, 0, index, data.Length); - index += data.Length; + // write data - encode numeric text directly into bitArray + index = PlainTextToBinaryNumeric(Text, bitArray, index); return index;Add this overload to stream bits (outside this hunk):
private static int PlainTextToBinaryNumeric(string plainText, BitArray bitArray, int index) { // Triplets -> 10 bits for (int i = 0; i < plainText.Length - 2; i += 3) { #if HAS_SPAN var dec = int.Parse(plainText.AsSpan(i, 3), NumberStyles.None, CultureInfo.InvariantCulture); #else var dec = int.Parse(plainText.Substring(i, 3), NumberStyles.None, CultureInfo.InvariantCulture); #endif index = DecToBin(dec, 10, bitArray, index); } // Remainder int rem = plainText.Length % 3; if (rem == 2) { #if HAS_SPAN var dec = int.Parse(plainText.AsSpan(plainText.Length - 2, 2), NumberStyles.None, CultureInfo.InvariantCulture); #else var dec = int.Parse(plainText.Substring(plainText.Length - 2, 2), NumberStyles.None, CultureInfo.InvariantCulture); #endif index = DecToBin(dec, 7, bitArray, index); } else if (rem == 1) { #if HAS_SPAN var dec = int.Parse(plainText.AsSpan(plainText.Length - 1, 1), NumberStyles.None, CultureInfo.InvariantCulture); #else var dec = int.Parse(plainText.Substring(plainText.Length - 1, 1), NumberStyles.None, CultureInfo.InvariantCulture); #endif index = DecToBin(dec, 4, bitArray, index); } return index; }QRCoder/QRCodeGenerator/DataSegment.cs (2)
30-37: Docstring mentions chaining, but class no longer supports it.Remove “Chains to the next segment” to avoid confusion.
- /// Writes this data segment to an existing BitArray at the specified index for a specific QR code version. - /// Chains to the next segment if present. + /// Writes this data segment to an existing BitArray at the specified index for a specific QR code version.
51-57: Docstring refers to chained segments; adjust wording.GetBitLength should describe only this segment.
- /// Calculates the total bit length for this segment when encoded for a specific QR code version. - /// Includes the length of all chained segments. + /// Calculates the total bit length for this segment when encoded for a specific QR code version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
QRCoder/QRCodeGenerator.cs(2 hunks)QRCoder/QRCodeGenerator/AlphanumericDataSegment.cs(1 hunks)QRCoder/QRCodeGenerator/AlphanumericEncoder.cs(3 hunks)QRCoder/QRCodeGenerator/ByteDataSegment.cs(1 hunks)QRCoder/QRCodeGenerator/DataSegment.cs(2 hunks)QRCoder/QRCodeGenerator/NumericDataSegment.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
QRCoder/QRCodeGenerator/AlphanumericDataSegment.cs (3)
QRCoder/QRCodeGenerator.cs (7)
QRCodeGenerator(14-1028)QRCodeGenerator(19-21)DataSegment(126-138)EncodingMode(749-764)GetCountIndicatorLength(892-952)BitArray(996-1005)DecToBin(874-884)QRCoder/QRCodeGenerator/DataSegment.cs (6)
QRCodeGenerator(3-59)DataSegment(8-58)DataSegment(24-27)GetBitLength(57-57)WriteTo(37-37)BitArray(44-49)QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (2)
AlphanumericEncoder(8-99)WriteToBitArray(76-98)
QRCoder/QRCodeGenerator/ByteDataSegment.cs (1)
QRCoder/QRCodeGenerator.cs (8)
QRCodeGenerator(14-1028)QRCodeGenerator(19-21)DataSegment(126-138)EncodingMode(749-764)GetCountIndicatorLength(892-952)BitArray(996-1005)DecToBin(874-884)IsValidISO(973-985)
QRCoder/QRCodeGenerator/NumericDataSegment.cs (2)
QRCoder/QRCodeGenerator.cs (7)
QRCodeGenerator(14-1028)QRCodeGenerator(19-21)DataSegment(126-138)EncodingMode(749-764)GetCountIndicatorLength(892-952)BitArray(996-1005)DecToBin(874-884)QRCoder/QRCodeGenerator/DataSegment.cs (6)
QRCodeGenerator(3-59)DataSegment(8-58)DataSegment(24-27)GetBitLength(57-57)WriteTo(37-37)BitArray(44-49)
QRCoder/QRCodeGenerator/DataSegment.cs (5)
QRCoder/QRCodeGenerator.cs (3)
DataSegment(126-138)EncodingMode(749-764)BitArray(996-1005)QRCoder/QRCodeGenerator/AlphanumericDataSegment.cs (2)
WriteTo(46-61)GetBitLength(29-37)QRCoder/QRCodeGenerator/ByteDataSegment.cs (3)
WriteTo(76-100)BitArray(191-197)GetBitLength(59-67)QRCoder/QRCodeGenerator/NumericDataSegment.cs (3)
WriteTo(46-63)BitArray(72-113)GetBitLength(29-37)QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (2)
BitArray(60-65)GetBitLength(48-51)
QRCoder/QRCodeGenerator.cs (5)
QRCoder/QRCodeGenerator/NumericDataSegment.cs (3)
NumericDataSegment(8-64)NumericDataSegment(19-22)BitArray(72-113)QRCoder/QRCodeGenerator/AlphanumericDataSegment.cs (2)
AlphanumericDataSegment(8-62)AlphanumericDataSegment(19-22)QRCoder/QRCodeGenerator/ByteDataSegment.cs (3)
ByteDataSegment(12-101)ByteDataSegment(46-52)BitArray(191-197)QRCoder/QRCodeGenerator/DataSegment.cs (1)
BitArray(44-49)QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (1)
BitArray(60-65)
QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (2)
QRCoder/QRCodeGenerator/DataSegment.cs (2)
GetBitLength(57-57)BitArray(44-49)QRCoder/QRCodeGenerator.cs (2)
BitArray(996-1005)DecToBin(874-884)
⏰ 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 3.1
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Core 2.1
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: additional-tests
🔇 Additional comments (6)
QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (2)
43-51: Bit-length computation is correct and minimal.Formula
(n/2)*11 + (n&1)*6matches QR alphanumeric spec. LGTM.
62-65: Good delegation to writer without extra allocations.Pre-sizing via GetBitLength and writing in place avoids temporary arrays.
QRCoder/QRCodeGenerator/AlphanumericDataSegment.cs (2)
29-37: Correct total length calculation.4 (mode) + count-indicator + AlphanumericEncoder data length. Matches spec.
46-61: Write order and streaming encode look good.Mode → count (chars) → data via writer; no intermediate allocations. LGTM.
QRCoder/QRCodeGenerator.cs (2)
129-137: Segment factory switch is clear and extensible.Cleanly routes to Numeric/Alphanumeric/Byte segments; good cohesion.
1010-1029: ToBitArray helper preserves MSB→LSB and supports prefixes.Pre-sizing and delegating to CopyToBitArray is correct. LGTM.
Co-authored-by: Günther Foidl <[email protected]>
Co-authored-by: Günther Foidl <[email protected]>
Yes, the text encoding in 1.x is a bit botched. I'll fix in a separate PR. This PR will retain the exact same encoding algorithm as previous versions. |
2 participants
Enhance the DataSegment class to support multiple encoding modes and streamline the CreateDataSegment method by utilizing specialized segment classes, improving code structure and readability. Encoding algorithm of text has not changed.
Summary by CodeRabbit