Skip to content
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

Change how UTCInstant gets Clock #246

Open
ubamrein opened this issue Sep 16, 2020 · 1 comment
Open

Change how UTCInstant gets Clock #246

ubamrein opened this issue Sep 16, 2020 · 1 comment

Comments

@ubamrein
Copy link
Contributor

For testing purposes it is neat to overwrite the clock of UTCInstant. Currently, the used clock is static, which brings its various problems with it. Also the function setClock overwrites the clock for all future instances, which might lead to unexpected behaviour in case of forgotten resetClock. The latter can be prevented by using the proper Disposable feature of the UTCInstant class.

However, we should overthink the architecture and discuss the issue here, rather than in #215 since it does not pose an immediate risk and should no prevent he PR from merging (it only appears in unit-tests and integration-tests).

@ineiti
Copy link
Collaborator

ineiti commented Sep 17, 2020

Another code-smell is the fact that static variables are kind of a hidden global variable. And those are never good, because they make it harder to modularize and test things. If for example you want to test clock-skews of different clients, you have to take care that you don't overwrite the clocks from the different clients and the backend.

In addition to that, UTCInstant has a double-function:

  • getting the actual time and date
  • converting different time/duration representations from/to each other

The minimum change IMO is to:

  • change private static Clock to protected static Clock
  • move implements Closeable, setClock, resetClock and close from UTCInstant to UTCITest
  • make UTCITest only available in the tests

This would make sure that nobody calls setClock in the main code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants