Always return same instance of CurrentDate, CurrentTime, or CurrentDateTime#1467
Always return same instance of CurrentDate, CurrentTime, or CurrentDateTime#1467njr-11 wants to merge 3 commits into
Conversation
| } | ||
|
|
||
| // Internal implementation of single instance obtained from CurrentDate.now() | ||
| class CurrentDateInstance implements CurrentDate<Object> { |
There was a problem hiding this comment.
No - for two reasons. Java does not allow private on top level classes. Even if it did allow that, the CurrentDate class needs access to it, so the default access level of package-private is what we want.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
make it inner, static, private then
Java does not allow private inner classes on interfaces. That does not compile.
I understand CS might not be used here, but there also is https://checkstyle.org/checks/design/onetoplevelclass.html#Description to consider.
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 CurrentDateInstance is only used by CurrentDate (which is why it would have made sense as a private static inner class if Java allowed that on interfaces) and there is no value in any other classes using it, it seems appropriate to isolate it to the same file as CurrentDate in this case.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
But this will not prevent using it by other classes (in the package I mean), will it?
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.
|
|
||
| @Test | ||
| void testCurrentDateEqualsCurrentDate() { | ||
| assertEquals(CurrentDate.now(), |
There was a problem hiding this comment.
I would use the assertj on it.
There was a problem hiding this comment.
That's fine, I switched it for you.
|
I did think of one improvement to add: making the constructor private to enforce that only the single static instance can ever be obtained. This is covered in commit 0a0b0f0 |
| return "LOCAL DATE"; | ||
| } | ||
| }; | ||
| return (CurrentDate<T>) CurrentDateInstance.instance; |
There was a problem hiding this comment.
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 ...
4 participants
Currently we are creating a new instance of CurrentDate (and likewise for CurrentTime/CurrentDateTime) every time the TemporalExpression.localDate or CurrentDate.now method is invoked, such that CurrentDate.now() is unequal to CurrentDate.now() even though both represent the same datetime expression,
LOCAL DATE.