Skip to content

Refactor small maintainability issues in transformations, fake-value resolution and Irish PPSN generation#1871

Closed
tawratnibir wants to merge 5 commits into
datafaker-net:mainfrom
tawratnibir:refactor/phase3-int05
Closed

Refactor small maintainability issues in transformations, fake-value resolution and Irish PPSN generation#1871
tawratnibir wants to merge 5 commits into
datafaker-net:mainfrom
tawratnibir:refactor/phase3-int05

Conversation

@tawratnibir

Copy link
Copy Markdown

Summary

This pull request makes a small set of maintainability-focused refactorings:

  • Replaces raw Class usage in JavaObjectTransformer.apply() with Class<?> and typed instanceof handling.
  • Removes an unreachable dotIndex == -1 branch from the private FakeValuesService.resolveFakerObjectAndMethod() helper.
  • Removes a misleading expression-resolution comment that referred to indexOf() while the code uses contains("}").
  • Extracts the Irish PPSN weighted checksum calculation into a private helper method.

Why

These changes address small maintainability issues found during source inspection. They improve type clarity, remove unreachable control-flow noise, align comments with the current implementation, and separate checksum calculation from the high-level PPSN generation workflow.

The changes are intentionally small and avoid public API or generic-contract redesign.

Refactoring approach

The refactoring was kept local to the affected implementation details:

  • JavaObjectTransformer.apply() now uses Class<?> to represent an unknown class type more safely.
  • FakeValuesService.resolveFakerObjectAndMethod() now reflects the verified private helper call contract more directly.
  • FakeValuesService.resolveExpression() no longer contains a stale implementation-specific comment.
  • IrishIdNumber.generateValid() now delegates weighted checksum accumulation to calculateWeightedSum(...), while preserving the existing weights, suffix handling, multiplier, checksum character calculation and output format.

Validation

The following validation was performed:

  • JavaObjectTransformerTest and JavaObjectTransformerConstructorTest passed.
  • FakeValuesServiceTest passed.
  • IrishIdNumberTest passed.
  • Full Maven verification passed with:
./mvnw -Dgpg.skip=true clean verify

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.52%. Comparing base (2f057eb) to head (7383fed).
⚠️ Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
...tafaker/transformations/JavaObjectTransformer.java 50.00% 0 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1871      +/-   ##
============================================
+ Coverage     92.45%   92.52%   +0.06%     
- Complexity     3557     3563       +6     
============================================
  Files           345      346       +1     
  Lines          7025     7050      +25     
  Branches        685      685              
============================================
+ Hits           6495     6523      +28     
+ Misses          366      364       -2     
+ Partials        164      163       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tawratnibir

Copy link
Copy Markdown
Author

Thanks for maintaining DataFaker.

I noticed this PR contains a few small maintainability refactorings in separate areas. I kept them small and behaviour-preserving, but I am happy to split this into separate smaller PRs if that would better match the project’s contribution style.

Validation run locally:

  • JavaObjectTransformerTest and JavaObjectTransformerConstructorTest passed.
  • FakeValuesServiceTest passed.
  • IrishIdNumberTest passed.
  • Full Maven verification passed with ./mvnw -Dgpg.skip=true clean verify.

Please let me know if you would prefer this PR to be narrowed to one logical change.

@kingthorin

Copy link
Copy Markdown
Collaborator

I think it's okay as-is. Yes it's multiple ummm topics but it isn't 100s of lines of change either.

I'll try to review this afternoon, in my experience others are generally quiet on the weekends with this project.

@bodiam

bodiam commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Thanks for your contribution. I'm not sure how this PR is solving anything, and I don't think it's a good idea to move around stuff as a first PR while not maintaining this library. As such, closing this PR.

@bodiam bodiam closed this Jul 5, 2026
@tawratnibir

Copy link
Copy Markdown
Author

Thank you for reviewing the PR and for the feedback.

I understand the concern. This was submitted as a small maintainability-focused refactoring, but I respect that it may not fit the project’s preferred contribution direction.

I appreciate your time, and I will keep this feedback in mind for any future contribution by starting with a smaller, more clearly scoped issue or maintainer-approved change.

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.

4 participants