Skip to content

Conversation

@Shane32
Copy link
Owner

@Shane32 Shane32 commented Oct 12, 2025

Building on #670 towards multi-mode support

Summary by CodeRabbit

  • New Features
    • Improved QR code generation with enhanced ECI (extended character encoding) handling for more reliable international character support across versions.
  • Refactor
    • Simplified and consolidated how segment data is serialized into the final bit stream, improving maintainability and clarifying data flow.

…WriteTo methods, simplifying bit array construction.
@Shane32 Shane32 self-assigned this Oct 12, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 12, 2025

📝 Walkthrough

Walkthrough

Moves segment bit-array construction into DataSegment by adding a constructor and methods (ToBitArray, WriteTo). QRCodeGenerator now calls segment.ToBitArray(version) and the former private helper BuildBitArrayFromSegment is removed.

Changes

Cohort / File(s) Summary of changes
DataSegment API extension
QRCoder/QRCodeGenerator/DataSegment.cs
Added public DataSegment(EncodingMode encodingMode, int characterCount, BitArray data, EciMode eciMode); added public BitArray ToBitArray(int version) to produce a complete segment bit array; added public int WriteTo(BitArray bitArray, int startIndex, int version) to serialize ECI, mode, count and data into an existing BitArray.
Generator callsite refactor
QRCoder/QRCodeGenerator.cs
Replaced calls to BuildBitArrayFromSegment(segment, version) with segment.ToBitArray(version) and removed the private BuildBitArrayFromSegment(DataSegment, int) helper and its implementation.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant QRCodeGenerator
  participant DataSegment

  Caller->>QRCodeGenerator: Generate(...)
  QRCodeGenerator->>DataSegment: ToBitArray(version)
  activate DataSegment
  DataSegment->>DataSegment: WriteTo(new BitArray, 0, version)
  DataSegment-->>QRCodeGenerator: BitArray (segment)
  deactivate DataSegment
  QRCodeGenerator->>QRCodeGenerator: Combine segment bit arrays into payload
  QRCodeGenerator-->>Caller: Final QR code payload
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

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.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title Check ✅ Passed The title accurately summarizes the key change by indicating the refactor of the BuildBitArrayFromSegment method, which is the primary focus of the pull request, without including unnecessary details.
✨ 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 build_bitarray

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 12, 2025 22:38
@Shane32 Shane32 added this to the 1.7.1 milestone Oct 12, 2025
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

🧹 Nitpick comments (1)
QRCoder/QRCodeGenerator/DataSegment.cs (1)

86-90: ECI assignment values are correct; add validation for future ≥128 values
Verified EciMode enum matches the QR spec (Default=0, Iso8859_1=3, Iso8859_2=4, Utf8=26) and all values <128. Recommend adding a guard or documentation note in DataSegment.cs noting that ECI values ≥128 require 16/24-bit encoding per ISO/IEC 18004.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ab04613 and afefd63.

📒 Files selected for processing (2)
  • QRCoder/QRCodeGenerator.cs (1 hunks)
  • QRCoder/QRCodeGenerator/DataSegment.cs (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/QRCodeGenerator/DataSegment.cs (1)
QRCoder/QRCodeGenerator.cs (6)
  • BitArray (991-1000)
  • BitArray (1010-1051)
  • BitArray (1074-1150)
  • DecToBin (868-878)
  • 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). (6)
  • GitHub Check: Test .NET Framework 4.6.2
  • GitHub Check: Test .NET 6.0 Windows
  • GitHub Check: Test .NET 5.0 Windows
  • GitHub Check: Test .NET Core 3.1
  • GitHub Check: Test .NET Core 2.1
  • GitHub Check: additional-tests
🔇 Additional comments (3)
QRCoder/QRCodeGenerator.cs (1)

119-119: LGTM!

Clean refactoring that moves the bit array construction logic into the DataSegment type. The new instance method approach follows good OO principles by keeping behavior with data.

QRCoder/QRCodeGenerator/DataSegment.cs (2)

35-48: LGTM!

The constructor properly initializes all DataSegment fields including the new EciMode parameter.


62-72: LGTM!

The method correctly allocates a BitArray of the required size and delegates the serialization logic to WriteTo.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@Shane32 Shane32 changed the title Refactor BuildBitArrayFromDataSegment Refactor BuildBitArrayFromSegment Oct 12, 2025
@Shane32 Shane32 added the refactoring Refactoring of code without changes to functionality label Oct 13, 2025
@Shane32 Shane32 merged commit dd5596f into master Oct 13, 2025
8 checks passed
@Shane32 Shane32 deleted the build_bitarray branch October 13, 2025 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactoring Refactoring of code without changes to functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants