CodeSOD: Synchronize Your Clocks

Back when it was new, one of the great features" of Java was that it made working with threads easy". Developers learning the language were encouraged to get a grip right on threads right away, because that was the new thing which would make their programs so much better.

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."

Another thing that's hard in Java is working with dates and times.

Larisa inherited some code which wanted to be able to check the current system time in a threadsafe fashion. They weren't doing anything fancy- no timezones, no formatting, just getting the Unix Timestamp off the clock. If you're thinking to yourself, It's just a read operation and there's no need to bring threads into this at all," you obviously didn't write today's code block.

The right way to do this in Java 8+, would be to use the built-in java.time objects, but in older versions of Java you might need to do something like this:

long currentTime = System.currentTimeMillis();

But that doesn't involve any design patterns, any synchronized code blocks to protect against multiple threads, and simply isn't Enterprise enough.

public class Clock { private static Clock sfClock = null; protected static synchronized void register(Clock testClock) { sfClock = testClock; } public static synchronized Clock getIt() { if (sfClock == null) { sfClock = new Clock(); } return sfClock; } public static long now() { return getIt().nowImpl(); } protected long nowImpl() { return System.currentTimeMillis(); }}

This is an attempt to implement the Singleton pattern, which is the go to pattern for people to use, because it's the easiest to understand and implement and doubles as what is basically a global variable.

You'll note that there's no constructor, since there's no internal state, so there's no point in making this a singleton.

getIt will create an instance if there isn't one, but you can also supply an instance via register. You might think that the developer put some thought into how this class would be tested, but again- there's no internal state or even any internal logic. You could inherit from Clock to make a MockClock that could be used in testing, but that is a long hill to climb to justify this.

The real genius, though, is that our ugly getIt method doesn't ever have to be directly invoked. Instead, now does that for you. Clock.now() will call getIt, which gets an instance of Clock, then invoke nowImpl, the actual implementation of our now method.

For bonus points, the reason Larisa found this was that there are a lot of threads in this program, and they're tying timestamps to actions on the regular, so the fact that getIt is synchronized was actually killing performance.

None of this code is necessary. It's an over-engineered solution to a problem nobody actually had.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!