-
Notifications
You must be signed in to change notification settings - Fork 36
Always return same instance of CurrentDate, CurrentTime, or CurrentDateTime #1467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,17 +37,26 @@ public interface CurrentDate<T> extends TemporalExpression<T, LocalDate> { | |
| * | ||
| * @return an expression representing the current date. | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| static <T> CurrentDate<T> now() { | ||
| return new CurrentDate<>() { | ||
| @Override | ||
| public Class<LocalDate> type() { | ||
| return LocalDate.class; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "LOCAL DATE"; | ||
| } | ||
| }; | ||
| return (CurrentDate<T>) CurrentDateInstance.instance; | ||
| } | ||
| } | ||
|
|
||
| // Internal implementation of single instance obtained from CurrentDate.now() | ||
| class CurrentDateInstance implements CurrentDate<Object> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make it private?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No - for two reasons. Java does not allow
This comment was marked as resolved.
Sorry, something went wrong.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Java does not allow
This project does use checkstyle, but the OneTopLevelClass check is not enabled. I looked over that check (thanks for posting the direct link) and it does seem like a nice guideline to follow in a lot of cases but I do not see how it would add value here. Given that the
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree, it doesn't prevent other classes in the package from using it. Mostly, it just keeps it out of their way and isolates it to the only place that cares about it. |
||
| static final CurrentDate<?> instance = new CurrentDateInstance(); | ||
|
|
||
| private CurrentDateInstance() { | ||
| } | ||
|
|
||
| @Override | ||
| public Class<LocalDate> type() { | ||
| return LocalDate.class; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "LOCAL DATE"; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure I'm in the minority here, but honestly I kinda prefer an unnecessary instantiation to an unchecked cast. Yes, I get that it's perfectly sound here due to type argument erasure but even so ...