-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Refactor conversion of text to data segments #670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughRefactors QR code generation into a segmented flow: input is converted to a new DataSegment, CapacityTables can compute/validate the minimum version, a BitArray is built from the segment, and final QR construction uses that BitArray. Public APIs unchanged; CapacityTables gains a public TryCalculateMinimumVersion. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant QR as QRCodeGenerator
participant Cap as CapacityTables
Caller->>QR: GenerateQrCode(plainText, eccLevel, [version], options)
QR->>QR: CreateDataSegment(plainText, forceUtf8, utf8BOM, eciMode)
alt no fixed version
QR->>Cap: TryCalculateMinimumVersion(segment, eccLevel)
Cap-->>QR: minVersion / no-fit
alt minVersion found
QR->>QR: DetermineVersion(segment, eccLevel, null) -> minVersion
else no-fit
QR-->>Caller: throw DataTooLongException
end
else fixed version provided
QR->>QR: DetermineVersion(segment, eccLevel, fixedVersion) -> validate or throw
end
QR->>QR: BuildBitArrayFromSegment(segment, version)
QR->>QR: GenerateQrCode(bitArray, eccLevel, version)
QR-->>Caller: QRCodeData
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes 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 |
There was a problem hiding this 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
🧹 Nitpick comments (1)
QRCoder/QRCodeGenerator/CapacityTables.cs (1)
119-143: Public method uses a private type; restrict visibility or adjust APIThis public method’s signature exposes the private DataSegment type. Even though the containing class is private, the declared public modifier is misleading and may trigger accessibility inconsistencies. Since it’s only used internally, make the method non-public to avoid confusion.
Apply:
- public static bool TryCalculateMinimumVersion(DataSegment segment, ECCLevel eccLevel, out int version) + internal static bool TryCalculateMinimumVersion(DataSegment segment, ECCLevel eccLevel, out int version)Alternatively, keep it public but change the parameter to an internal/public DTO not tied to a private type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
QRCoder/QRCodeGenerator.cs(4 hunks)QRCoder/QRCodeGenerator/CapacityTables.cs(1 hunks)QRCoder/QRCodeGenerator/DataSegment.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
QRCoder/QRCodeGenerator/DataSegment.cs (2)
QRCoder/QRCodeGenerator.cs (8)
QRCodeGenerator(19-21)DataSegment(126-132)EncodingMode(777-792)BitArray(169-198)BitArray(1025-1034)BitArray(1044-1085)BitArray(1108-1184)GetCountIndicatorLength(920-980)QRCoder/QRCodeGenerator/CapacityTables.cs (1)
QRCodeGenerator(3-420)
QRCoder/QRCodeGenerator.cs (3)
QRCoder/QRCodeGenerator/DataSegment.cs (2)
DataSegment(35-41)GetBitLength(48-53)QRCoder/QRCodeGenerator/CapacityTables.cs (2)
CapacityTables(9-419)TryCalculateMinimumVersion(127-143)QRCoder/Exceptions/DataTooLongException.cs (3)
DataTooLongException(6-30)DataTooLongException(14-17)DataTooLongException(26-29)
QRCoder/QRCodeGenerator/CapacityTables.cs (2)
QRCoder/QRCodeGenerator.cs (1)
DataSegment(126-132)QRCoder/QRCodeGenerator/DataSegment.cs (2)
DataSegment(35-41)GetBitLength(48-53)
⏰ 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 Framework 4.6.2
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET Core 2.1
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: additional-tests
🔇 Additional comments (4)
QRCoder/QRCodeGenerator.cs (4)
114-121: Segmented flow: clear and cohesiveCreating a DataSegment, determining version, then building the BitArray improves clarity and reusability. LGTM.
138-164: Version determination path looks solidGood separation of concerns and error path via Throw helper. Once HasEciMode is fixed, min-version calculation should be accurate.
Please re-run tests for boundary payloads (near version capacity) after fixing HasEciMode to ensure no false DataTooLongException.
169-178: BitArray assembly consistent with spec and segment modelWrites ECI (when present), mode, count, then data. This matches GetBitLength once HasEciMode is corrected. LGTM.
Also applies to: 197-197
407-407: Good: discard unused parameter to silence warningSafe and explicit suppression.
…g for QR code capacity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
QRCoder/QRCodeGenerator/CapacityTables.cs (1)
119-167: LGTM! Solid implementation of segment-based version calculation.The method correctly accounts for version-dependent mode and count indicator lengths by grouping versions into three ranges (1-9, 10-26, 27-40) where these lengths remain constant. The logic is sound, edge cases are handled properly, and the Try-pattern return convention is idiomatic.
Optional: Reduce code duplication.
The three loops share nearly identical structure. Consider extracting a helper method:
+private static bool TryFindVersionInRange(DataSegment segment, ECCLevel eccLevel, int startVersion, int endVersion, int bitLengthVersion, out int version) +{ + var segmentBitLength = segment.GetBitLength(bitLengthVersion); + for (version = startVersion; version <= endVersion; version++) + { + var eccInfo = GetEccInfo(version, eccLevel); + if (eccInfo.TotalDataBits >= segmentBitLength) + { + return true; + } + } + version = 0; + return false; +} + public static bool TryCalculateMinimumVersion(DataSegment segment, ECCLevel eccLevel, out int version) { - // Versions 1-9: Count indicator length is constant within this range - var segmentBitLength = segment.GetBitLength(1); - for (version = 1; version <= 9; version++) - { - var eccInfo = GetEccInfo(version, eccLevel); - // Check if this version has enough capacity for the segment's total bits - if (eccInfo.TotalDataBits >= segmentBitLength) - { - return true; - } - } - - // Versions 10-26: Count indicator length is constant within this range - segmentBitLength = segment.GetBitLength(10); - for (version = 10; version <= 26; version++) - { - var eccInfo = GetEccInfo(version, eccLevel); - // Check if this version has enough capacity for the segment's total bits - if (eccInfo.TotalDataBits >= segmentBitLength) - { - return true; - } - } - - // Versions 27-40: Count indicator length is constant within this range - segmentBitLength = segment.GetBitLength(27); - for (version = 27; version <= 40; version++) - { - var eccInfo = GetEccInfo(version, eccLevel); - // Check if this version has enough capacity for the segment's total bits - if (eccInfo.TotalDataBits >= segmentBitLength) - { - return true; - } - } - - version = 0; - return false; + return TryFindVersionInRange(segment, eccLevel, 1, 9, 1, out version) + || TryFindVersionInRange(segment, eccLevel, 10, 26, 10, out version) + || TryFindVersionInRange(segment, eccLevel, 27, 40, 27, out version); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/QRCodeGenerator/CapacityTables.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/QRCodeGenerator/CapacityTables.cs (1)
QRCoder/QRCodeGenerator/DataSegment.cs (2)
DataSegment(35-41)GetBitLength(48-53)
⏰ 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 Core 3.1
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Core 2.1
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: additional-tests
Prep to support multi-mode QR codes
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation