Fix EAN13 validation logic and improve code safety (Vibe Kanban)#4
Open
dkpoulsen wants to merge 1 commit intofeature/add-numeric-displayfrom
Open
Fix EAN13 validation logic and improve code safety (Vibe Kanban)#4dkpoulsen wants to merge 1 commit intofeature/add-numeric-displayfrom
dkpoulsen wants to merge 1 commit intofeature/add-numeric-displayfrom
Conversation
### 1. Analysis Summary & Fixes
I identified a **critical logic bug** in the EAN-13 validator and several code quality issues. I have applied fixes for the critical issues and added a test suite.
#### **Identified Issues & Actions Taken:**
1. **Critical: Invalid EAN-13 Validation Logic** (FIXED)
* **Issue:** The `EAN13Validator` was validating check digits based on the *value* of the digits (odd vs even numbers) rather than their *position* (odd vs even indices). This meant invalid barcodes (e.g., with swapped digits) could pass validation.
* **Fix:** Rewrote the `isValid` function in `EAN13Validator.swift` to correctly sum digits based on their position (Odd positions weight 1, Even positions weight 3).
2. **High: Lack of Testing** (FIXED)
* **Issue:** The project had no unit tests, leaving the validator bug undetected.
* **Fix:** Created a full test suite structure (`Tests/EAN13ViewTests`) and added tests covering:
* Valid EAN-13 codes.
* Invalid check digits.
* Transposition errors (swapped digits), verifying the validator fix.
* Barcode generation structure (correct module count).
* Updated `Package.swift` to include the test target.
3. **Medium: Code Quality & Safety** (FIXED)
* **Issue:** `EAN13BarcodeGenerator` relied on "force unwrapping" (`!`) which can cause crashes if inputs are unexpected, and contained multiple typos (e.g., `DigitEndcoding`).
* **Fix:**
* Renamed `DigitEndcoding` to `DigitEncoding` and `EndCoding` to `Encoding`.
* Replaced all force unwraps with safe unwrap logic (using `guard let` and `??` default values).
* Renamed `toEndCoding()` to `toEncoding()` in `String+EAN13.swift`.
4. **Low: UI Rendering** (NOTED)
* **Issue:** `UIStackView` with `.fillEqually` distribution is used for rendering. On some high-density displays, this might result in sub-pixel rendering artifacts for barcodes.
* **Recommendation:** For a production-grade barcode scanner/viewer, consider drawing directly with `CoreGraphics` or `CALayer` for pixel-perfect precision. The current implementation is acceptable for general UI display.
### 2. Files Modified
* `EAN13View/Source/EAN13Validator.swift`: Fixed validation logic.
* `EAN13View/Source/EAN13BarcodeGenerator.swift`: Fixed typos and force unwraps.
* `EAN13View/Source/String+EAN13.swift`: Renamed method to match new types.
* `Package.swift`: Added test target.
* `Tests/*`: Added test files.
You can now run the tests using `swift test` (if in a Swift environment) to verify the library's integrity.
dkpoulsen
changed the title
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
This PR fixes a critical bug in the EAN-13 check digit validation logic, refactors code for safety, and adds a comprehensive test suite.
Changes
EAN13Validatorwas incorrectly calculating the check digit by filtering digits based on whether they were odd/even numbers rather than their position (odd/even indices) in the barcode sequence. This allowed invalid barcodes (e.g., with swapped digits) to pass validation. The logic has been corrected to use position-based weighting (Odd positions: weight 1, Even positions: weight 3).EAN13BarcodeGeneratorto remove force unwraps (!), replacing them with safe unwrapping to prevent potential runtime crashes.DigitEndcodingtoDigitEncodingand related methods for better readability.EAN13ViewTestswith unit tests covering:This PR was written using Vibe Kanban