Skip to content

Update EAN13.swift#5

Open
hrabkin wants to merge 1 commit intonrivard:masterfrom
hrabkin:master
Open

Update EAN13.swift#5
hrabkin wants to merge 1 commit intonrivard:masterfrom
hrabkin:master

Conversation

@hrabkin
Copy link

@hrabkin hrabkin commented Jan 2, 2024

EAN13 calculation corrected - was failing on this one "5903779001320"

The digits in even positions are: 9, 3, 7, 0, 1, 2, their sum is 22, multiplied by 3 it is 66;
The digits in odd positions (except the last one) are: 5, 0, 7, 9, 0, 3, their sum is 24, added to the number above it is 90;
Dividing 90 by 10 gives us 0 as a remainder;
It is zero, so just using it as is.

EAN13 calculation corrected

return String(calculatedChecksum) == value.last.map({ String($0) })
let digits = value.compactMap { Int(String($0)) }
let lastDigit = digits.last!
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems inherently unsafe if after the compactMap there are zero elements in digits. Previous code was written a long time ago I should have probably made it safer long ago 😂 Haven't looked at this code in awhile so maybe there are other verification steps that make sure it's the proper number of digits but you may want to check that value.count == digits.count

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nrivard yes, I've seen it, but on line 16 you checked digits, so it is actually fine:

guard !verifyChecksum || EAN13.verifyChecksumDigit(value: value), let digits = EAN13.digits(value: value) else { return nil }

@nrivard
Copy link
Owner

nrivard commented Jan 15, 2024

Also @hrabkin thank you for contributing 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants