Conversation
PR Summary
|
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1559 +/- ##
============================================
- Coverage 92.58% 92.45% -0.13%
- Complexity 3315 3329 +14
============================================
Files 329 330 +1
Lines 6510 6550 +40
Branches 632 643 +11
============================================
+ Hits 6027 6056 +29
- Misses 335 339 +4
- Partials 148 155 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| public class IrishIdNumberTest { | ||
| private static final Logger LOG = Logger.getLogger(IrishIdNumberTest.class.getName()); | ||
| private final IrishIdNumber irishIdNumber = new IrishIdNumber(); | ||
| private final Faker faker = new Faker(new Locale("ie", "IE")); |
There was a problem hiding this comment.
@89snake89 Wait, isn't "ga" the language code of Ireland (Irish Gaelic)?
While "ie" is the code of international auxiliary language "Interlingue".
There was a problem hiding this comment.
Ireland has 2 official languages English (mostly used) and Irish Gaelic (very few people can speak it)
There was a problem hiding this comment.
Gemini's summary:
The ISO 639-1 language code for Irish (Ireland) is ga. The corresponding locale code, which combines the language and country, is ga-IE. This indicates the Irish language as spoken in Ireland.
There was a problem hiding this comment.
@89snake89 In this specific test language is not used at all, so it's not critical. :)
BUT
To avoid confusion, let's use
- either
new Locale("en", "IE")(which means English language) - or
new Locale("ga", "IE")(which means Gaelic language)
But not new Locale("ie", "IE") because "ie" stands for "Interlingue" language, not English.
There was a problem hiding this comment.
You are correct, my mistake it must be new Locale("en", "IE")
| return ppsn.toString(); | ||
| } | ||
|
|
||
| public boolean validateAndCheckModulo23(String ppsn) { |
There was a problem hiding this comment.
I would make this method non-public (so called "package private").
Or wait, why this method is needed at all?
There was a problem hiding this comment.
I was thinking to have a separate method to divide and test the validity of the PPSN
There was a problem hiding this comment.
It's a good idea to split code into smaller methods and test them separately.
But I assume you don't expect your users to call this method, right? @89snake89
But then remove keyword "public" from such methods.
- Then it becomes "package-private", and you can still can call this method from tests.
- But end-users cannot call this method.
| @Override | ||
| public String generateInvalid(final BaseProviders faker) { | ||
| // Generate 7 digits | ||
| Random random = new Random(); |
There was a problem hiding this comment.
Usually we don't use new Random() in DataFaker.
Instead, please use faker methods, e.g.
String digits = faker.number().digits(7);It's good for reproducible result.
There was a problem hiding this comment.
Good Idea! Thank you
|
|
||
| @Override | ||
| public String generateValid(final BaseProviders faker) { | ||
| Random random = new Random(); |
There was a problem hiding this comment.
Instead of new Random(), please use faker methods, e.g.
String digits = faker.number().digits(7);
boolean addSuffix = faker.bool().bool();
String suffix = addSuffix ? faker.options().option(allowedSuffixes) : "";etc.
| String digits = faker.number().digits(7); | ||
| // Append always invalid character (es: 'Z') | ||
| return digits + "Z"; |
There was a problem hiding this comment.
You could combine the two and return directly avoiding the intermediate store of digits
| boolean addSuffix = random.nextBoolean(); // 50% chance | ||
| String suffix = addSuffix ? allowedSuffixes[random.nextInt(allowedSuffixes.length)] : ""; |
There was a problem hiding this comment.
Here too, you could inline the random Boolean and skip the intermediate assignment
| for (int d : digits) { | ||
| ppsn.append(d); | ||
| } |
There was a problem hiding this comment.
Digits is a string right? Just append it directly.
| return ppsn.toString(); | ||
| } | ||
|
|
||
| public boolean validateAndCheckModulo23(String ppsn) { |
| } | ||
| sum += (c - '0') * weights[i]; | ||
| } | ||
| // if there is a suffic incude it in the checksum |
Co-authored-by: Rick M <[email protected]>
Co-authored-by: Rick M <[email protected]>
Co-authored-by: Rick M <[email protected]>
Co-authored-by: Rick M <[email protected]>
Co-authored-by: Rick M <[email protected]>
Co-authored-by: Rick M <[email protected]>
|
Seems to need a spotless apply |
4 participants
Irish PPSN ID Number