7
\$\begingroup\$

I have created a StopWatch class to measure the performance of any code. I use this StopWatch code in any of my multithreading projects as well.

public static class StopWatch {

    public static ConcurrentHashMap<Long, Long> histogram = new ConcurrentHashMap<Long, Long>();

    /**
     * Creates an instance of the timer and starts it running.
     */
    public static StopWatch getInstance() {
        return new StopWatch();
    }

    private long end = -1;
    private long interval = -1;
    private final long start;

    private StopWatch() {
        start = interval = currentTime();
    }

    /**
     * Returns in milliseconds the amount of time that has elapsed since the timer was created. If the
     * <code>stop</code> method has been invoked, then this returns instead the elapsed time between the creation of
     * the timer and the moment when <code>stop</code> was invoked.
     * 
     * @return duration it took
     */
    public long getDuration() {
        long result = 0;

        final long startTime = start;
        final long endTime = isTimerRunning() ? currentTime() : end;

        result = nanoToMilliseconds(endTime - startTime);

        boolean done = false;
        while (!done) {
            Long oldValue = histogram.putIfAbsent(result, 1L);
            if (oldValue != null) {
                done = histogram.replace(result, oldValue, oldValue + 1);
            } else {
                done = true;
            }
        }

        return result;
    }

    /**
     * Returns in milliseconds the amount of time that has elapsed since the last invocation of this same method. If
     * this method has not previously been invoked, then it is the amount of time that has elapsed since the timer
     * was created. <strong>Note</strong> that once the <code>stop</code> method has been invoked this will just
     * return zero.
     * 
     * @return interval period
     */
    public long getInterval() {
        long result = 0;

        final long startTime = interval;
        final long endTime;

        if (isTimerRunning()) {
            endTime = interval = currentTime();
        } else {
            endTime = end;
        }

        result = nanoToMilliseconds(endTime - startTime);

        return result;
    }

    /**
     * Stops the timer from advancing. This has an impact on the values returned by both the
     * <code>getDuration</code> and the <code>getInterval</code> methods.
     */
    public void stop() {
        if (isTimerRunning()) {
            end = currentTime();
        }
    }

    /**
     * What is the current time in nanoseconds?
     * 
     * @return returns back the current time in nanoseconds
     */
    private long currentTime() {
        return System.nanoTime();
    }

    /**
     * This is used to check whether the timer is alive or not
     * 
     * @return checks whether the timer is running or not
     */
    private boolean isTimerRunning() {
        return (end <= 0);
    }

    /**
     * This is used to convert NanoSeconds to Milliseconds
     * 
     * @param nanoseconds
     * @return milliseconds value of nanoseconds
     */
    private long nanoToMilliseconds(final long nanoseconds) {
        return nanoseconds / 1000000L;
    }
}

I wanted to review this piece of code. Any suggestions or improvements I can make here?

\$\endgroup\$
  • \$\begingroup\$ Not code-related, but have you considered other microbenchmark tools such as JMH, Caliper, or even something written by our very own @rolfl, YAuB? \$\endgroup\$ – h.j.k. Dec 22 '15 at 6:36
7
\$\begingroup\$

Just a few little things:

  • A public static getInstance() method is commonly used in the singleton pattern. However, you always create a new StopWatch. At least you should name it to createNewInstance(), if you want to keep the method at all.

  • Many of your comments are unnecessary. For example the comment before isTimerRunning() doesn't help to understand the code. If you are generating documentation, they might be required, if not, omit such comments.

  • The result variable in getInterval() is not required. You can just return the result like this:

    return nanoToMilliseconds(endTime - startTime);
    

    This saves you two lines of code and makes the code clearer. The same applies to getDuration().

  • by using the ternary if operator, you can shorten getInterval() a bit more:

    public long getInterval() {  
        final long startTime = interval;
        final long endTime = isTimerRunning() ? currentTime() : end;
        interval = endTime;
        return nanoToMilliseconds(endTime - startTime);;
    }
    

    The assignment interval = endTime now also happens when the StopWatch has already been stopped but this isn't a problem.

| improve this answer | |
\$\endgroup\$
5
\$\begingroup\$

This is more than a stop watch: it's a stop watch with a global histogram. I suggest to split this to multiple classes:

  • stop watch
  • histogram
  • stop watch with global histogram

Global state is usually undesirable. It would be better to have a stop watch factory class, that has a histogram, and provides stop watch instances that share that histogram. This would avoid global state.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

A small suggestion: in nanoToMilliseconds() you could write the value 1000000L as 1_000_000L. It doesn't change anything, except that it may be easier to read that this is a one followed by six zeroes.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.