• Purple (unregistered)

    This article genuinely brought a smile to my face.

  • (nodebb)

    It’s an over-engineered solution to a problem nobody actually had. It's what cómputers and their engineers mostly do, after all.

    Addendum 2020-06-03 06:50: WONTFIX

  • witchdoctor (unregistered)

    I'm betting there was some kind of guideline for abstracting out stateful interactions with the outside world (IO, querying things that change by themselves, etc) because someone well-intentioned wanted to make things easily testable.

    This is the kind of code you get after that person and their successor left when rules-lawyers that don't understand what the rule is for are trying to implement things. There are clean ways to make tests with predefined timestamps and other simulated values. This is not the way.

  • Hans (unregistered)

    And the nowImpl ignores the result of getIt - totally destroying any point this code may have had.

  • Naomi (unregistered)

    If mockability was really the concern, they could have used an approach like this:

    public interface Clock {

    int timestamp();

    Clock NOW = System::currentTimeMillis();

}

    Reaching for java.time to get the epoch timestamp would be overkill, IMO.

  • Hanneman (unregistered)

    Reminds me of: https://medium.com/@webseanhickey/the-evolution-of-a-software-engineer-db854689243

  • On the Clock (unregistered)

    There's no justification for the singleton (and I'm not even sure they did that right, since the variable they're storing it in isn't volatile, and synchronizing on the static method is inefficient). getCurrentTimeMillis (or new Date()) is thread safe. (Using the old approach isn't wrong here, btw, unless you need accuracy that only java.time will give you, and that is rarely the case in normal business systems. Some parts of java.util.Date are WTFy but timestamps aren't one of them.)

    But having a boundary class to allow you to mock out the current time is useful, it makes writing tests for time based interactions far easier. I've pushed an Environment interface for things like 'get the current time', 'get the file system' and stuff like that which you want to go outside your codebase when deployed, but you want to be able to mock for testing. Obviously the production implementation of that just delegates to the relevant system call though.

  • (nodebb)

    The Real WTF today is Remy and his explanation. Replacing system call that obtains current date/time by something mockable is actually very good idea if the real code contains lots of time-based logic. I would even say it's necessary - how do you write test for code that is supposed to handle one-year expiration? Or accounting code that depends on length of current month?

    Of course, the implementation is WTF indeed: besides synchronized block (which is performance killer), it is also not desirable to mock date globally. Usually, ThreadLocal is better choice.

  • Brian (unregistered)
    There's no justification for the singleton.

    That statement is almost always true. Of all the design patterns, singletons are the one I most despise. It's like someone's coding standard said "no global variables" for many well-justified reasons, so they just decided to obfuscate it in a class, and now they've got the same global-variable dangers compounded by the twisted logic that's required to make a global variable masquerade as a static class.

    Yes, yes, I know there are a handful of cases where it is the best solution to a problem, but that's greatly dwarfed by the cases where it's decidedly not but people use it anyway because their knowledge is greater than their understanding.

  • WTFGuy (unregistered)

    @Remy

    Well, concurrency is hard. Or, to put it another way, “I had a problem, so I decided to use threads. prhave twI Now o oblems.”

    I actually laughed out loud at that one. Bravo Sir, well played!

  • Naomi (unregistered) in reply to Brian

    I think there's one relatively common case where singletons are appropriate, and that's when the singleton is a stateless implementation of some interface. There's three reasons:

    • Clients can (should) rely on the API; only factories and the like need to know about the singleton.
    • Since it's specifically a stateless implementation, it's more like a global constant than a global variable.
    • It's really easy to test that a factory picks the right implementation.
  • adhdeveloper (unregistered)

    When I started programming I though enterprise meant the application was well tested, easy to debug, could scale seamlessly and solved a lot of corner cases people didn't think of. Now that I am 7+ years in I realize what enterprise actually means is that the company used unnecessary complexity to hide their incompetence. This code is very enterprise.

  • (nodebb)

    I disagree on there being no justification for the singleton. If you have some sort of unique external resource that is used by multiple spots in the program it's generally better to control that from a single point in your program--and since there's only one resource the controller should be a singleton.

    Time isn't such a resource, though!

  • Solw Purpose Of Visit (unregistered)

    Using multithreading in user space to access information in kernel space?

    Wow. I don't think we've seen this level of pure idiocy before. This one's a masterpiece.

  • ooOOooGa (unregistered) in reply to Naomi

    Indeed. The problem with global variables is that they are global read/write. Anything that is globally accessible and has both read and write causes problems because you can never be sure that what you put into it is what you are going to get back out again.

    Global state is fine as long as you are only doing one of the two. Global read only (a constant) is fine. Global write only (a singleton logging utility) is also fine. You may even be able to do something like a singleton data object that reads environment configuration at application launch and only allows the program to read those environment variables (a mostly read only scenario).

    But anything with global access and read/write access is going to cause headaches. Files on disk actually fall into this category too. Which is why those cause headaches and people generally prefer using databases (specifically ones that have transaction support) for data storage instead. And why file locking support was added to the OS.

  • Jim (unregistered)

    Not the best code ever seen, granted, but even a casual read shows there is some point to the exercise: protected static synchronized void register(Clock testClock)

    Namely the ability to slot in an alternative impl, presumably during unit testing where using the actual wall clock time would be undesirable. Exactly why with the newer java.time stuff there is indeed a Clock class allowing for just that flexibility.

    Use of a singleton rather than DI is of course still pure evil tho...

  • sizer99 (google)

    I used to work at a place where the coding standards pretty much guaranteed this. You know the drill, they'd had a serious error due to synchronization race conditions at some point in the past so the boss (not the sharpest technical guy) mandated all new classes be fully protected even if it was ludicrous, as in this case. That was C++, but everything from this one still applies.

  • some ron (unregistered)

    Well... actually the whole thing is sound and not over-engineered at all. Apart from some details, there is no better way to do it. Here's the situation where this thing makes perfect sense:

    • your code has some functions that are dependent on the current time (like do something differently at night or on the weekend or in the last 3 days before the end of the month)
    • those functions may be deeply nested, so to get the current time you have to -- either pass it down through every function (which is tedious and might not even be possible, if there are callbacks, that just don't support passing an additional parameter) -- or use a static function in some object (like System.currentTimeMillis() or Clock.now())
    • you want to test your code reproducibly, so you have to set the current time artificially (otherwise you'd get different results if you run the test at different times or you could only test the function that tests the behavior on the 29th of February every 4 years)
    • you do not have a dependency injection framework (or need the time in places where you can't easily access it)

    Therefore you use a Clock.now() instead of System.currentTimeMillis(), but somehow Clock.now() should delegate to System.currentTimeMillis() in production, but to some artificial clock, that you control yourself while you are testing. Your test would look like: TestClock tc=new TestClock(); Clock.register(tc); tc.setNow("friday 22:00"); executeFridayNightTest();

    The clock should be set at exactly once at the start of each test or at the start of the program. Clock.now() should be just sfClock.nowImpl(), then getIt is not necessary anymore. This avoids synchronization, but gives you all the benefits of a flexible clock.

  • Kari Ikonen (github)

    There is valid reasons for such pattern, regarding testing and simulation at least. Thus stating this to be "pure idiotic" is WTF by itself. Sure not best implementation, but certainly there is lot of cases when thread local (yeah, real fun trying to pass state to threads created in contexts, which you can't control) or dependency injection (cannot change dynamically on the fly, and again how to pass that state can get tricky) simply won't work. Especially this simulation case can require changing such clock, globally and dynamically on-demand, on running system.

  • On the Clock (unregistered) in reply to Brian

    That statement is almost always true. Of all the design patterns, singletons are the one I most despise

    Singletons make sense if

    • It's a representation of a genuinely single external resource, and
    • You can't just assign it in a static constructor because you need lazy load semantics, or you're using some dependency management container that makes it a pain to use that. (Though if you're using e.g. Spring it will handle the singletonness for you.)

    I do agree that a lot of uses of the singleton pattern are just hiding global variables though and I much prefer to have those in a nice obvious place so it's clear what they are. In OO languages that tends to be a static class called AppState or something with static fields/properties for the singular services.

  • On the Clock (unregistered) in reply to Naomi

    I think there's one relatively common case where singletons are appropriate, and that's when the singleton is a stateless implementation of some interface

    If it's stateless then it doesn't matter whether it's a singleton or not because any instance is switchable with any other.

  • Pedro (unregistered) in reply to Hans
    And the nowImpl ignores the result of getIt - totally destroying any point this code may have had.

    No, nowImpl is not Static, it's a member of the class.

  • doubtingposter (unregistered)

    This looks exactly like the hill Remy doesn't want us to climb - this was written as a wrapper around the System.currentTimeMillis() so that it can be replaced for testing application wide. Whether or not that's a good idea to do is an entirely different question from what is being suggested is the WTF.

  • WTFGuy (unregistered) in reply to On the Clock

    @On the Clock ref

    If it's stateless then it doesn't matter whether it's a singleton or not because any instance is switchable with any other.

    That's true as far as it goes. But from a resource consumption perspective it might be really stupid to expensively generate and discard instance after instance when for just a little more fairly simple code you can allocate & initialize it exactly once then reuse it for the life of the application.

    Just like classes, properties, and methods should be given the least scope necessary (prefer private over the various intermediate flavors over public), objects should be as few a number as possible. Use static where you can, singleton where static doesn't quite work, and multiple instances only when necessary.

    The fact that both public and "multiple instances" are by far the most common use case statistically doesn't alter the fact that they should not be the default assumption for access scope or number.

    A high quality tight design exposes (or creates) only what's necessary.

  • (nodebb)

    AS posted: design for testability..... Perhaps somewhere in your code you are testing "are two things in the same day" - if you have direct calls all over the system, then it is near impossible. With this, just have a custom implementation of clock that you can set from your test routine, register it - and you can do all of the time testing you want without modification of any code....

    Does anyone have a "simpler" approach that addresses all of the above [note: I am talking about approach, not exact implementation as there are many near equivilants]

  • Rein (unregistered)

    Since the parameter is called 'testClock' it was probably put in place to test business logic which uses timestamps. A much better approach is to accept time as input parameter, rather than just taking the current time (System.currentTimeMillis). It looks a bit silly when you know it's only ever going to be called with the current time, but it makes your code way more testable. Also, it often just makes sense, take Certificate.isValid() or Certificate.isValidAt(time). The second one is way more flexible.

  • iPAtPYC (unregistered)
    Comment held for moderation.

Leave a comment on “Synchronize Your Clocks”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article