Skip to content

Move id number objects to final fields to decrease number of object creations#379

Merged
snuyanzin merged 1 commit intodatafaker-net:mainfrom
snuyanzin:ids
Sep 23, 2022
Merged

Move id number objects to final fields to decrease number of object creations#379
snuyanzin merged 1 commit intodatafaker-net:mainfrom
snuyanzin:ids

Conversation

@snuyanzin
Copy link
Collaborator

The PR reduces creation of id related objects

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2022

Codecov Report

Merging #379 (71b511d) into main (d87ad81) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #379      +/-   ##
============================================
- Coverage     93.68%   93.68%   -0.01%     
  Complexity     1951     1951              
============================================
  Files           215      215              
  Lines          3946     3942       -4     
  Branches        385      385              
============================================
- Hits           3697     3693       -4     
  Misses          149      149              
  Partials        100      100              
Impacted Files Coverage Δ
src/main/java/net/datafaker/base/IdNumber.java 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bodiam
Copy link
Contributor

bodiam commented Sep 23, 2022

Wouldn't this increase the number of object creations in most cases? If it's really important, you could create them lazily. Also, is the number of object creations really relevant?

(Don't get me wrong, I don't mind the PR at all of course, I'm just wondering if it improves things really)

@snuyanzin snuyanzin merged commit 71b511d into datafaker-net:main Sep 23, 2022
@snuyanzin
Copy link
Collaborator Author

oops, sorry I noticed your comment after merging...
I thought about lazy... I will do it in a separate step where I want to move all id fakers into a separate package
Thanks for the feedback

@bodiam
Copy link
Contributor

bodiam commented Sep 23, 2022

oops, sorry I noticed your comment after merging...
I thought about lazy... I will do it in a separate step where I want to move all id fakers into a separate package
Thanks for the feedback

No issue at all. I don't think it has to be lazy at all, it's only 6 objects, and creating them is super fast. Using lazy might result into concurrency issues etc, , so I don't think I'd go for that.

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.

3 participants