Problem
I’ve made a little library where I add some functionalities from time to time that I miss in the Java standard library.
I am sad that the Java library for example has no stop watch class that allows me to measure time without the need to deal with details of a time library.
When I write library functionalities, I have the following requirements to myself:
- Keep it simple, stupid. The code has to be on point and should only do what is really needed.
- The code has to be very readable, even if this leads to some more code lines.
- The task has to be solved efficiently. Memory and calculation time should not be wasted.
- If the standard library or other stuff like that is needed, only use stable, long-lasting and modern technologies, prioritized in this order.
So this class was born. What do you think about it? Did I meet my own requirements?
import java.time.Instant;
import java.time.Duration;
public class StopWatch {
private Instant startDate;
private Instant stopDate;
public StopWatch() {
reset();
}
public void start() {
if (startDate == null) {
startDate = Instant.now();
}
}
public void stop() {
if (stopDate == null && startDate != null) {
stopDate = Instant.now();
}
}
public void reset() {
startDate = null;
stopDate = null;
}
public long getMilli() {
if (startDate != null && stopDate != null) {
Duration duration = Duration.between(startDate, stopDate);
long nanos = duration.getSeconds() * 1000000000 + duration.getNano();
return nanos / 1000000;
}
return 0;
}
}
Purpose
The code following now is not part of the code that should be reviewed. Some people wanted me to show the use case and that it is:
I have a class thats objects calculate prime numbers. I want to measure how much time it needs to calculate these numbers, so therefore i need a stopwatch class for preventing overbloating my code with date-time-apis of my programming language.
Main.java
public class Main {
public static void main(String[] args) throws Exception {
StopWatch sw = new StopWatch();
sw.start();
PrimeNumberCalculator pnc = new PrimeNumberCalculator();
pnc.setCurrentNumber(10000);
StringBuilder sb = new StringBuilder();
for (int i = 0; i < 1000; i++) {
pnc.calculateNextPrime();
sb.append((i+1) + ";" + pnc.getCurrentPrimeNumber() + "n");
}
sw.stop();
System.out.println(sb.toString());
System.out.println(sw.getMilli());
}
}
PrimeNumberCalculator.java
public class PrimeNumberCalculator {
private long currentNumber;
private long currentPrimeNumber;
public PrimeNumberCalculator(long currentNumber) {
this.currentNumber = currentNumber;
}
public PrimeNumberCalculator() {
this(3);
currentPrimeNumber = 2;
}
public long getCurrentPrimeNumber() {
return currentPrimeNumber;
}
public void setCurrentNumber(long currentNumber) {
this.currentNumber = currentNumber;
}
public void calculateNextPrime() {
boolean isPrime;
do {
isPrime = true;
for (long i = 2; i < currentNumber; i++) {
if (currentNumber % i == 0 && currentNumber != 2) {
isPrime = false;
break;
}
}
if (isPrime) {
currentPrimeNumber = currentNumber;
}
currentNumber++;
} while (!isPrime);
}
}
The class is also needed in my own Minesweeper implementation, where players can play against the time and be ranked in criterias of time needed.
Solution
For a library class, JavaDocs should be given (well, here the interface is so small and well-named that one can easily guess the tasks of the methods, but anyway).
As already said, the getMilli() method should be replaced with a getDuration() one. Your conversion to millis not only includes ugly computation code, but also cripples the available resolution. And if some user wants milliseconds, there’s the Duration.toMillis() method.
Silently ignoring a method call when it has been done in an invalid state should never be the correct approach. Calling start() when the stop watch has already been started is a programming error so it should not just “do nothing.”
It is unclear what the code is used for. Is it for debugging or profiling? Do you use the result in program logic?
If it is for profiling, one of the undesired qualities of using stop watces is that it litters production code with variables that are not related to the application logic. I would rather have a static stop watch that stores it’s state in a thread local so that I only have to call StopWatch.start(); and StopWatch.stop(duration -> log.debug("Execution took {}", duration)); to make it work. Pass an identifier if you need several watches running. Probably have the implementation check a system property (or Spring profile, etc) to see if it should run or do nothing so that I don’t have to change the code when it goes to production (or add even more undesired boolean variables to decide if profiling should be done).
- As mentioned in the comments,
startTimeis more sensible thanstartDate - in testing, I found
Instantto have resolution issues; see also https://stackoverflow.com/a/20689231/313768 . For short times,nanoTime()is more appropriate. - Do not offer a
getMilli; that isn’t the responsibility of this class. Just offer a genericDuration. - Consider simplifying your usage contract so that the start time is immutable, and the stop time is set on closure of an
AutoCloseable.
Suggested
import java.time.Duration;
public class StopWatch implements AutoCloseable {
private final long startTime = System.nanoTime();
private Long stopTime = null;
public void stop() {
stopTime = System.nanoTime();
}
public Duration getLength() {
return Duration.ofNanos(stopTime - startTime);
}
@Override
public void close() {
stop();
}
}
///////
import static java.lang.System.out;
public class Main {
public static void main(String[] args) {
StopWatch watch = new StopWatch();
try (watch) {
out.println("Hello world");
}
out.printf("That took %.1f us%n", watch.getLength().toNanos()/1e3);
}
}
From the design perspective – have you considered making the class a pure function? I’d see its api like (keeping naming similar):
long milis = StopWatch.measureInMilis(() -> {... measured logic as runnable...})
Note that in this stateless approach:
- your code becomes almost impossible to use incorrectly – it usually is a sign of better design (client code is unable to call start or stop twice)
- Implementation becomes simpler – you don’t need ifs for checking edge cases
- Code becomes easier to test – you don’t need to reset the clock between tests (speaking of, you didn’t need it in the first place – as you could just create new instance per test)
- Single usage of utility means lesser coupling with the client code – if you ever wanted to refactor this solution it would be less things to change
StopWatchbecomes thread safe for free
Apart from the above please note that currently your code has big external implicit dependency – on a system clock which makes is generally difficult, and slow, to test – but it might be a conscious decision as this implicit character of the dependency makes this code easier to use (you don’t need to set this dependency up). If some business logic were to depend on that it would also make this logic more difficult to test – which might be unwanted side effect in bigger applications,
Also the final calculation seems rather clunky – Duration has a method toMillis() that would spare you unnecessary manual calculations.
Summing it all up, consider alternative implementation:
public class StopWatch {
public static long measureInMillis(Runnable operation) {
Instant start = Instant.now();
operation.run();
Instant end = Instant.now();
return Duration.between(start, end).toMillis();
}
}
A broader view of development scenario, skip it for a narrow approach of code reviewing. The code snippet from one of the answers to this question could be a unit test that is an easy maintainable alternative to technical documentation or live documentation linking requirements with the developed solution to the extent that any change of the implementation that breaks the requirements is a compile time error. It worth mentioning the Test-driven development (T.D.D.) approach of software development for its concern over the efficiency and maintainability despite its low relevance for the subject at hand.
With only purpose of having a complete depict of a possible alternative consider following implementation for the requirements listed by early mentioned answer.
public class StopWatch {
private ChronoUnit chronoUnit;
private NanoWatch watcher;
private AtomicReference<Long> startNanosHolder;
private AtomicReference<Long> endNanosHolder;
private boolean timing;
private static class Builder {
private ChronoUnit chronoUnit;
private boolean timing;
private Builder() {}
public StopWatch.Builder timing(boolean timingEnabled) {
this.timing = timingEnabled;
return this;
}
public StopWatch.Builder timeMeasureOf(ChronoUnit chronoUnit) {
this.chronoUnit = chronoUnit;
return this;
}
public StopWatch build() {
StopWatch stopWatch = new StopWatch(this.timing);
stopWatch.chronoUnit = this.chronoUnit;
return stopWatch;
}
}
public static StopWatch.Builder builder() {
return new StopWatch.Builder();
}
private StopWatch(boolean timing) {
this.chronoUnit = ChronoUnit.NANOS;
this.timing = timing;
this.watcher = NanoWatch.valueOf(this.timing);
this.startNanosHolder = new AtomicReference<Long>();
this.endNanosHolder = new AtomicReference<Long>();
}
public StopWatch measureOf(ChronoUnit unit) {
this.chronoUnit = unit;
return this;
}
public StopWatch start() {
if (startNanosHolder.get() != null) {
return this;
}
this.watcher.time(startNanosHolder);
return this;
}
public StopWatch stop() {
if (startNanosHolder.get() == null || endNanosHolder.get() != null) {
return this;
}
this.watcher.time(endNanosHolder);
return this;
}
private boolean isTiming() {
return startNanosHolder.get() != null && endNanosHolder.get() == null;
}
public Optional<Long> measure() {
if (!this.timing) {
return Optional.empty();
}
long endNanos = this.isTiming() ? System.nanoTime() : this.endNanosHolder.get();
return this.watcher.measure(startNanosHolder.get(), endNanos).map(this::convert);
}
public Long convert(Duration toConvert) {
return ChronoUnitConverter.valueOf(this.chronoUnit)
.map(chronoUnitConverter -> chronoUnitConverter.convert(toConvert)).orElse(null);
}
private enum ChronoUnitConverter {
TO_SECONDS(ChronoUnit.SECONDS) {
public long convert(Duration toConvert) {
return toConvert.toMillis();
}
}
, TO_MILIS(ChronoUnit.MILLIS) {
public long convert(Duration toConvert) {
return toConvert.toMillis();
}
}
, TO_NANOS(ChronoUnit.NANOS) {
public long convert(Duration toConvert) {
return toConvert.toMillis();
}
};
private ChronoUnit toConvertTo;
private ChronoUnitConverter(ChronoUnit toConvertTo) {
this.toConvertTo = toConvertTo;
}
public long convert(Duration toConvert) {
return toConvert.toSeconds();
}
public static Optional<ChronoUnitConverter> valueOf(ChronoUnit chronoUnit) {
Optional<ChronoUnitConverter> toReturn = Optional.empty();
for (ChronoUnitConverter converter : ChronoUnitConverter.values()) {
if (chronoUnit == converter.toConvertTo) {
toReturn = Optional.of(converter);
break;
}
}
return toReturn;
}
}
private enum NanoWatch {
TIMELESS()
, TIMER() {
public void time(AtomicReference<Long> timeNanosHolder) {
timeNanosHolder.set(System.nanoTime());
}
public Optional<Duration> measure(Long startNanos, Long endNanos) {
return Optional.of(Duration.of(endNanos - startNanos, ChronoUnit.NANOS));
}
};
public void time(AtomicReference<Long> timeNanosHolder) { }
public Optional<Duration> measure(Long startNanos, Long endNanos) {
return Optional.empty();
}
public static NanoWatch valueOf(boolean timed) {
return (timed) ? TIMER : TIMELESS;
}
}
}