Prep DataSegment for multi mode QR codes #683
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ength calculations and writing methods
Shane32
added
the
refactoring
📝 WalkthroughWalkthroughAdds length- and slice-aware APIs for QR alphanumeric and numeric segments. New static GetBitLength(int, version) and WriteTo(text, offset, length, bitArray, bitIndex, version) enable partial encoding. Existing instance methods delegate to the new statics. AlphanumericEncoder updated to accept lengths and index/count ranges. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Encoder as AlphanumericEncoder/NumericDataSegment
participant BitArr as BitArray
Caller->>Encoder: GetBitLength(textLength, version)
Note over Encoder: Compute mode + count + data bits\nbased on provided length
Caller->>BitArr: Allocate/ensure capacity
rect rgba(230,245,255,0.5)
Note over Caller,Encoder: Slice-aware write
Caller->>Encoder: WriteTo(text, offset, length, BitArr, bitIndex, version)
Encoder->>BitArr: Write mode + count
loop Encode slice
Encoder->>BitArr: Write data bits for [offset .. offset+length)
end
Encoder-->>Caller: return newBitIndex
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
…hanumericDataSegment
…n AlphanumericEncoder
…stency and clarity
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
QRCoder/QRCodeGenerator/AlphanumericDataSegment.cs (2)
34-49: Validate inputs; consider access modifier
- Add a guard for textLength >= 0 to avoid negative sizes.
- Minor: This is inside a private nested class; marking the method public is misleading. Consider making it private/internal for clarity.
Apply guards:
public static int GetBitLength(int textLength, int version) { + if (textLength < 0) throw new ArgumentOutOfRangeException(nameof(textLength)); int modeIndicatorLength = 4; int countIndicatorLength = GetCountIndicatorLength(version, EncodingMode.Alphanumeric); int dataLength = AlphanumericEncoder.GetBitLength(textLength); int length = modeIndicatorLength + countIndicatorLength + dataLength; return length; }Optionally adjust visibility:
-public static int GetBitLength(int textLength, int version) +internal static int GetBitLength(int textLength, int version)
63-87: Add argument validation and capacity assertions; consider visibility
- Validate text != null, offset/length range, bitArray != null, bitIndex >= 0, and version range.
- Optional: Debug.Assert capacity to prevent BitArray overflow.
- Minor: As above, consider internal/private instead of public for a method on a private class.
public static int WriteTo(string text, int offset, int length, BitArray bitArray, int bitIndex, int version) { + if (text is null) throw new ArgumentNullException(nameof(text)); + if (bitArray is null) throw new ArgumentNullException(nameof(bitArray)); + if (offset < 0) throw new ArgumentOutOfRangeException(nameof(offset)); + if (length < 0) throw new ArgumentOutOfRangeException(nameof(length)); + if (offset > text.Length - length) throw new ArgumentOutOfRangeException($"{nameof(offset)},{nameof(length)}"); + if (bitIndex < 0) throw new ArgumentOutOfRangeException(nameof(bitIndex)); + if ((version is 0) || (version < -4) || (version > 40)) throw new ArgumentOutOfRangeException(nameof(version)); + + // Optional: ensure destination has enough capacity in DEBUG builds + // Needed bits = 4 (mode) + countIndicator + AlphanumericEncoder data bits + int countIndicatorLength = GetCountIndicatorLength(version, EncodingMode.Alphanumeric); + int needed = 4 + countIndicatorLength + AlphanumericEncoder.GetBitLength(length); + System.Diagnostics.Debug.Assert(bitIndex + needed <= bitArray.Length, "BitArray too small for write."); // write mode indicator bitIndex = DecToBin((int)EncodingMode.Alphanumeric, 4, bitArray, bitIndex); // write count indicator - int countIndicatorLength = GetCountIndicatorLength(version, EncodingMode.Alphanumeric); bitIndex = DecToBin(length, countIndicatorLength, bitArray, bitIndex); // write data - encode alphanumeric text bitIndex = AlphanumericEncoder.WriteToBitArray(text, offset, length, bitArray, bitIndex); return bitIndex; }QRCoder/QRCodeGenerator/NumericDataSegment.cs (3)
34-49: Input validation; consider access modifier
- Guard against negative textLength.
- Minor: Public inside a private nested class can be confusing; consider internal/private.
public static int GetBitLength(int textLength, int version) { + if (textLength < 0) throw new ArgumentOutOfRangeException(nameof(textLength)); int modeIndicatorLength = 4; int countIndicatorLength = GetCountIndicatorLength(version, EncodingMode.Numeric); int dataLength = textLength / 3 * 10 + (textLength % 3 == 1 ? 4 : textLength % 3 == 2 ? 7 : 0); int length = modeIndicatorLength + countIndicatorLength + dataLength; return length; }
63-89: Rename ‘startIndex’ (text) to ‘offset’; add guards; optional capacity assert
- Avoid confusion with the other ‘startIndex’ (bit array index) in this class by using ‘offset’ (aligns with AlphanumericDataSegment).
- Validate inputs as in the alphanumeric variant.
-/// <param name="startIndex">The starting index in the text to encode from</param> +/// <param name="offset">The starting index in the text to encode from</param> /// <param name="length">The number of characters to encode</param> ... -public static int WriteTo(string text, int startIndex, int length, BitArray bitArray, int bitIndex, int version) +public static int WriteTo(string text, int offset, int length, BitArray bitArray, int bitIndex, int version) { - var index = bitIndex; + if (text is null) throw new ArgumentNullException(nameof(text)); + if (bitArray is null) throw new ArgumentNullException(nameof(bitArray)); + if (offset < 0) throw new ArgumentOutOfRangeException(nameof(offset)); + if (length < 0) throw new ArgumentOutOfRangeException(nameof(length)); + if (offset > text.Length - length) throw new ArgumentOutOfRangeException($"{nameof(offset)},{nameof(length)}"); + if (bitIndex < 0) throw new ArgumentOutOfRangeException(nameof(bitIndex)); + if ((version is 0) || (version < -4) || (version > 40)) throw new ArgumentOutOfRangeException(nameof(version)); + + // Optional: ensure destination capacity in DEBUG builds + int countIndicatorLength = GetCountIndicatorLength(version, EncodingMode.Numeric); + int needed = 4 + countIndicatorLength + (length / 3) * 10 + (length % 3 == 1 ? 4 : length % 3 == 2 ? 7 : 0); + System.Diagnostics.Debug.Assert(bitIndex + needed <= bitArray.Length, "BitArray too small for write."); + + var index = bitIndex; // write mode indicator index = DecToBin((int)EncodingMode.Numeric, 4, bitArray, index); // write count indicator - int countIndicatorLength = GetCountIndicatorLength(version, EncodingMode.Numeric); index = DecToBin(length, countIndicatorLength, bitArray, index); // write data - encode numeric text - index = PlainTextToBinaryNumeric(text, startIndex, length, bitArray, index); + index = PlainTextToBinaryNumeric(text, offset, length, bitArray, index); return index; }
107-149: Simplify loop: avoid mutating offset/length; compute remainder from loop indexImproves readability and reduces cognitive load without changing behavior.
-private static int PlainTextToBinaryNumeric(string plainText, int offset, int length, BitArray bitArray, int bitIndex) -{ - var endIndex = offset + length; - - // Process each group of three digits. - for (int i = offset; i < endIndex - 2; i += 3) - { - // Parse the next three characters as a decimal integer. +private static int PlainTextToBinaryNumeric(string plainText, int offset, int length, BitArray bitArray, int bitIndex) +{ + int end = offset + length; + + // Process each group of three digits. + int i = offset; + for (; i + 2 < end; i += 3) + { +#if HAS_SPAN - var dec = int.Parse(plainText.AsSpan(i, 3), NumberStyles.None, CultureInfo.InvariantCulture); + 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); + var dec = int.Parse(plainText.Substring(i, 3), NumberStyles.None, CultureInfo.InvariantCulture); #endif - // Convert the decimal to binary and store it in the BitArray. - bitIndex = DecToBin(dec, 10, bitArray, bitIndex); - offset += 3; - length -= 3; + bitIndex = DecToBin(dec, 10, bitArray, bitIndex); } - // Handle any remaining digits if the total number is not a multiple of three. - if (length > 0) // Two remaining digits are encoded in 7 bits; one remaining digit is encoded in 4 bits. + // Handle remaining 1–2 digits. + int rem = end - i; + if (rem > 0) // Two remaining digits are 7 bits; one digit is 4 bits. { #if HAS_SPAN - var dec = int.Parse(plainText.AsSpan(offset, length), NumberStyles.None, CultureInfo.InvariantCulture); + var dec = int.Parse(plainText.AsSpan(i, rem), NumberStyles.None, CultureInfo.InvariantCulture); #else - var dec = int.Parse(plainText.Substring(offset, length), NumberStyles.None, CultureInfo.InvariantCulture); + var dec = int.Parse(plainText.Substring(i, rem), NumberStyles.None, CultureInfo.InvariantCulture); #endif - bitIndex = DecToBin(dec, length == 2 ? 7 : 4, bitArray, bitIndex); + bitIndex = DecToBin(dec, rem == 2 ? 7 : 4, bitArray, bitIndex); } return bitIndex; }QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (1)
68-79: Add argument validation; optional capacity assert; consider param naming
- Validate plainText/codeText, index/count bounds, and non-negative codeIndex.
- Optional: Debug.Assert sufficient capacity for the write.
- Nit: Consider renaming parameters to offset/length for consistency with DataSegment.
-public static int WriteToBitArray(string plainText, int index, int count, BitArray codeText, int codeIndex) +public static int WriteToBitArray(string plainText, int index, int count, BitArray codeText, int codeIndex) { + if (plainText is null) throw new ArgumentNullException(nameof(plainText)); + if (codeText is null) throw new ArgumentNullException(nameof(codeText)); + if (index < 0) throw new ArgumentOutOfRangeException(nameof(index)); + if (count < 0) throw new ArgumentOutOfRangeException(nameof(count)); + if (index > plainText.Length - count) throw new ArgumentOutOfRangeException($"{nameof(index)},{nameof(count)}"); + if (codeIndex < 0) throw new ArgumentOutOfRangeException(nameof(codeIndex)); + + // Optional: ensure destination capacity in DEBUG builds + int needed = GetBitLength(count); + System.Diagnostics.Debug.Assert(codeIndex + needed <= codeText.Length, "BitArray too small for write."); // Process each pair of characters. while (count >= 2)Optional parameter rename for consistency:
-public static int WriteToBitArray(string plainText, int index, int count, BitArray codeText, int codeIndex) +public static int WriteToBitArray(string plainText, int offset, int length, BitArray codeText, int codeIndex) { - // Process each pair of characters. - while (count >= 2) + var index = offset; + var count = length; + // Process each pair of characters. + while (count >= 2)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
QRCoder/QRCodeGenerator/AlphanumericDataSegment.cs(2 hunks)QRCoder/QRCodeGenerator/AlphanumericEncoder.cs(2 hunks)QRCoder/QRCodeGenerator/NumericDataSegment.cs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (4)
QRCoder/QRCodeGenerator/NumericDataSegment.cs (3)
GetBitLength(29-32)GetBitLength(41-49)BitArray(98-105)QRCoder/QRCodeGenerator/AlphanumericDataSegment.cs (2)
GetBitLength(29-32)GetBitLength(41-49)QRCoder/QRCodeGenerator/ByteDataSegment.cs (2)
GetBitLength(59-67)BitArray(191-197)QRCoder/QRCodeGenerator.cs (1)
BitArray(996-1005)
QRCoder/QRCodeGenerator/AlphanumericDataSegment.cs (4)
QRCoder/QRCodeGenerator/NumericDataSegment.cs (5)
GetBitLength(29-32)GetBitLength(41-49)WriteTo(58-61)WriteTo(74-89)BitArray(98-105)QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (4)
GetBitLength(48-51)AlphanumericEncoder(8-98)BitArray(60-65)WriteToBitArray(78-97)QRCoder/QRCodeGenerator/DataSegment.cs (3)
GetBitLength(57-57)WriteTo(37-37)BitArray(44-49)QRCoder/QRCodeGenerator.cs (4)
GetCountIndicatorLength(892-952)EncodingMode(749-764)BitArray(996-1005)DecToBin(874-884)
QRCoder/QRCodeGenerator/NumericDataSegment.cs (4)
QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (2)
GetBitLength(48-51)BitArray(60-65)QRCoder/QRCodeGenerator/AlphanumericDataSegment.cs (4)
GetBitLength(29-32)GetBitLength(41-49)WriteTo(58-61)WriteTo(74-87)QRCoder/QRCodeGenerator/ByteDataSegment.cs (3)
GetBitLength(59-67)WriteTo(76-100)BitArray(191-197)QRCoder/QRCodeGenerator/DataSegment.cs (3)
GetBitLength(57-57)WriteTo(37-37)BitArray(44-49)
⏰ 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: additional-tests
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: Test .NET Core 2.1
🔇 Additional comments (8)
QRCoder/QRCodeGenerator/AlphanumericDataSegment.cs (2)
29-32: LGTM: Instance method delegates to static helperKeeps behavior while enabling slice-based workflows.
60-61: LGTM: Delegation preserves behaviorGood reuse of the new static overload.
QRCoder/QRCodeGenerator/NumericDataSegment.cs (3)
29-33: LGTM: Instance method delegates to staticMatches the pattern introduced for slice-aware APIs.
60-61: LGTM: Delegation preserves behaviorConsistent with alphanumeric implementation.
98-106: LGTM: Wrapper overload delegates to in-place writerKeeps legacy behavior intact while reusing new slice-aware logic.
QRCoder/QRCodeGenerator/AlphanumericEncoder.cs (3)
44-51: LGTM: Length-based bit length computationCorrect formula for alphanumeric groups.
62-64: LGTM: Reuses new length-based APIGood reuse; no behavior change.
44-51: All WriteToBitArray and GetBitLength callsites use the updated signatures. Repo-wide search confirmed no uses of the old overloads.
gfoidl
approved these changes
Oct 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor