Take the 2-minute tour ×
Stack Overflow is a question and answer site for professional and enthusiast programmers. It's 100% free, no registration required.

There is a question which was recently asked to me in an interview.

Problem: There is a class meant to profile the execution time of the code. The class is like:

Class StopWatch {

    long startTime;
    long stopTime;

    void start() {// set startTime}
    void stop() { // set stopTime}
    long getTime() {// return difference}

}

The client is expected to create an instance of the StopWatch and call methods accordingly. User code can mess up the use of the methods leading to unexpected results. Ex, start(), stop() and getTime() calls should be in order.

This class has to be "reconfigured" so that user can be prevented from messing up the sequence.

I proposed use of custom exception if stop() is called before start(), or doing some if/else checks, but interviewer was not satisfied.

Is there a design pattern to handle these kind of situations?

Edit: The class members and method implementations can be modified.

share|improve this question
    
Are you allowed to change the class? Did the interviewer indicate that he wanted to catch a programmer error statically (in compile time)? –  aioobe 23 hours ago
    
The three methods should remain intact and are only interface to the programmer. The class members and method implementation can change. –  Mohitt 23 hours ago
10  
You could solve it statically by introducing wrappers: StoppedStopWatch with start() method that returns a RunningStopWatch with a stop() method. (And then prevent any direct use of the underlying StopWatch, for instance by making it package private.) –  aioobe 22 hours ago
    
Did you elaborate on your proposed approaches during the interview beyond what you did in the question? Because I'd expect a reasonable interviewer to not be satisfied with so much left to be clarified, even if he/she is looking for either of those solutions. –  Dukeling 16 hours ago

9 Answers 9

up vote 10 down vote accepted

We commonly use StopWatch from Apache Commons StopWatch check the pattern how they've provided.

IllegalStateException is thrown when the stop watch state is wrong.

public void stop()

Stop the stopwatch.

This method ends a new timing session, allowing the time to be retrieved.

Throws:
    IllegalStateException - if the StopWatch is not running.

Straight forward.

share|improve this answer
13  
How exactly is this different from what the OP said? "I proposed use of custom exception if stop() is called before start(), ..., but interviewer was not satisfied." –  Slanec 22 hours ago
1  
vels4j, I guess this approach is exactly what Eran has proposed. Seems I was on right track but could not consolidate my answer. So I guess if there is a sequence to be maintained for method calls, we shall start with state-diagram and define the valid/invalid transitions. –  Mohitt 22 hours ago
1  
@Mohitt If I came know there is benchmark solution, I will use it without a question. –  vels4j 22 hours ago

First there is the fact that implementing an own Java profiler is a waste of time, since good ones are available (maybe that was the intention behind the question).

If you want to enforce the correct method order at compile time, you have to return something with each method in the chain:

  1. start() has to return a WatchStopper with the stop method.
  2. Then WatchStopper.stop() has to return a WatchResult with the getResult() method.

External Construction of those helper classes as well as other ways of accessing their methods have to be prevented of course.

share|improve this answer
1  
yes, looks like it is the right answer. –  AdamSkywalker 22 hours ago
1  
@AdamSkywalker The expected answer, but the right one? It depends. –  coredump 18 hours ago
3  
The design pattern in this case is to apply a state machine to the solution, where only valid transitions are possible at each state. –  Fuhrmanator 11 hours ago
1  
The only downside of this is the fragmentation of the state logic in varying classes. It's the part I don't like about the State Pattern from GoF. stackoverflow.com/a/30888746/1168342 keeps all the logic in a single class. –  Fuhrmanator 11 hours ago

With minor changes to the interface, you can make the method sequence the only one that can be called - even at compile time!

public class Stopwatch {
    public static RunningStopwatch createRunning() {
        return new RunningStopwatch();
    }
}

public class RunningStopwatch {
    private final long startTime;

    RunningStopwatch() {
        startTime = System.nanoTime();
    }

    public FinishedStopwatch stop() {
        return new FinishedStopwatch(startTime);
    }
}

public class FinishedStopwatch {
    private final long elapsedTime;

    FinishedStopwatch(long startTime) {
        elapsedTime = System.nanoTime() - startTime;
    }

    public long getElapsedNanos() {
        return elapsedTime;
    }
}

The usage is straightforward - every method returns a different class which only has the currently applicable methods. Basically, the state of the stopwatch is encapsuled in the type system.


In comments, it was pointed out that even with the above design, you can call stop() twice. While I consider that to be added value, it is theoretically possible to screw oneself over. Then, the only way I can think of would be something like this:

class Stopwatch {
    public static Stopwatch createRunning() {
        return new Stopwatch();
    }

    private final long startTime;

    private Stopwatch() {
        startTime = System.nanoTime();
    }

    public long getElapsedNanos() {
        return System.nanoTime() - startTime;
    }
}

That differs from the assignment by omitting the stop() method, but that's potentially good design, too. All would then depend on the precise requirements...

share|improve this answer
    
(If anyone is considering using this, note that this is a design answer, not a correct time class - it's not testable! Consider using a Clock instance being used internally so that the class can be easily tested, or at least DateTimeUtils.currentTimeMillis() if you're using Joda-time and not Java 8 yet.) –  Slanec 22 hours ago
4  
Nothing, and both calls are valid - I actually consider this an added value, both calls return a different FinishedStopwatch so that you can reuse one RunningStopwatch for multiple measurements. Now, indeed, while even a wrong usage of this would be most likely correct, it is possible to mess this up in some horrible way. If we want to shield even against that, we should omit the stop() instance and only provide the getElapsedNanos() once a running stopwatch instance is obtained. –  Slanec 22 hours ago
2  
This is probably what the interviewer meant, but it's not the right choice for production. –  usr 21 hours ago
1  
@usr It isn't? I use this pattern ... sometimes. It's useful for constructing complex objects, and I call it the Menu pattern (without any knowledge of whether it already has a name or not) and in the wild, it's used e.g. for Guava's MultimapBuilder. I'm still curious to hear why you'd think it's not suited for production. –  Slanec 20 hours ago
1  
Well, it's fine by itself but it is very verbose. In case of a simple stopwatch I think this is clearly worse. The API surface is much larger. Also, for a stopwatch you wouldn't use a builder pattern or "fluent style" because the operations that you invoke are only one at a time. There is never a need for new Stopwatch().start().stop(). The .NET stopwatch has idempotent operations. You can call stop multiple times, which is useful because often you want to ensure that the watch is stopped. You don't care about moving from running to stopped, you care about the end result only. –  usr 20 hours ago

Maybe he expected this 'reconfiguration' and question was not about method sequence at all:

class StopWatch {

   public static long runWithProfiling(Runnable action) {
      startTime = now;
      action.run();
      return now - startTime;
   }
}
share|improve this answer
    
AdamSkyWalker, I asked if he wants to measure execution time of threads, but he mentioned that he is looking for a simple approach in single threaded environment. –  Mohitt 22 hours ago
2  
@Mohitt But my answer is not about threads, Runnable is just an interface that gives the possibility to wrap method execution and measure spent time. It should be used in a single thread environment. –  AdamSkywalker 22 hours ago
1  
I think something like this is probably what the interviewer expected (though right now it is not syntactically correct). The best response would probably have been to provide several designs and discuss the advantages and disadvantages of each. –  Sebastian Reichelt 13 hours ago

Once given more thought

In hindsight it sounds like they were looking for the execute around pattern. They're usually used to do things like enforce closing of streams. This is also more relevant due to this line:

Is there a design pattern to handle these kind of situations?

The idea is you give the thing that does the "executing around" some class to do somethings with. You'll probably use Runnable but it's not necessary. Runnable makes the most sense and you'll see why soon. In your StopWatch class add some method like this

public long measureAction(Runnable r) {
    start();
    r.run();
    stop();
    return getTime();
}

You would thing call it like this

StopWatch stopWatch = new StopWatch();
Runnable r = new Runnable() {
    // Put some tasks here you want to measure.
};
long time = stopWatch.measureAction(r);

This makes it fool proof. You don't have to worry about handling stop before start or people forgetting to call one and not the other, etc. The reason Runnable is nice is because

  1. Standard java class, not your own or third party
  2. End users can put whatever they need in the Runnable to be done.

(If you were using it to enforce stream closing then you could put the actions that need to be done with a database connection inside so the end user doesn't need to worry about how to open and close it and you simultaneously force them to close it properly.)

If you wanted, you could make some StopWatchWrapper instead leave StopWatch unmodified. You could also make measureAction(Runnable) not return a time and make getTime() public instead.

The Java 8 way to calling it is even simpler

StopWatch stopWatch = new StopWatch();
long time = stopWatch.measureAction(() - > {/* Measure stuff here */});

Original answer

More hindsight edit: OP mentions in a comment,

"The three methods should remain intact and are only interface to the programmer. The class members and method implementation can change."

So the below is wrong because it removes something from the interface. (Technically, you could implement it as an empty method but that seems to be like a dumb thing to do and too confusing.) I kind of like this answer if the restriction wasn't there and it does seem to be another "fool proof" way to do it so I will leave it.

To me something like this seems to be good.

class StopWatch {

    private final long startTime;

    public StopWatch() {
        startTime = ...
    }

    public long stop() {
        currentTime = ...
        return currentTime - startTime;
    }
}

The reason I believe this to be good is the recording is during object creation so it can't be forgotten or done out of order (can't call stop() method if it doesn't exist).

One flaw is probably the naming of stop(). At first I thought maybe lap() but that usually implies a restarting or some sort (or at least recording since last lap/start). Perhaps read() would be better? This mimics the action of looking at the time on a stop watch. I chose stop() to keep it similar to the original class.

The only thing I'm not 100% sure about is how to get the time. To be honest that seems to be a more minor detail. As long as both ... in the above code obtain current time the same way it should be fine.

share|improve this answer

Throwing an exception when the methods are not called in the correct order is common. For example, Thread's start will throw an IllegalThreadStateException if called twice.

You should have probably explained better how the instance would know if the methods are called in the correct order. This can be done by introducing a state variable, and checking the state at the start of each method (and updating it when necessary).

share|improve this answer
    
You could also reuse startTime and for instance set it to -1 before the stop watch has been started. –  aioobe 22 hours ago
2  
Eran, it seems you are right. Perhaps he wanted specific answer with examples. Thanks for the suggestion. –  Mohitt 22 hours ago
    
@aioobe That's certainly an option, but I'm not sure it's enough, since you probably don't want to reset the start and end times when you stop the watch (since getTime wouldn't give you the correct time), so you won't be able to tell the different between a started watch and a stopped watch without adding a state variable. –  Eran 22 hours ago

I suggest something like:

interface WatchFactory {
    Watch startTimer();
}

interface Watch {
    long stopTimer();
}

It will be used like this

 Watch watch = watchFactory.startTimer();

 // Do something you want to measure

 long timeSpentInMillis = watch.stopTimer();

You can't invoke anything in wrong order. And if you invoke stopTimer twice you get meaningful result both time (maybe it is better rename it to measure and return actual time each time it invoked)

share|improve this answer

This can also be done with Lambdas in Java 8. In this case you pass your function to the StopWatch class and then tell the StopWatch to execute that code.

Class StopWatch {

    long startTime;
    long stopTime;

    private void start() {// set startTime}
    private void stop() { // set stopTime}
    void execute(Runnable r){
        start();
        r.run();
        stop();
    }
    long getTime() {// return difference}
}
share|improve this answer
    
Note that this code would also work without Lambdas with the less concise anonymous inner class syntax, or any implementation of Runnable for that matter (nested class, top-level class, etc.) –  Max Nanasy 8 hours ago

Presumably the reason for using a stopwatch is that the entity that's interested in the time is distinct from the entity that's responsible for starting and stopping the timing intervals. If that is not the case, patterns using immutable objects and allowing code to query a stop watch at any time to see how much time has elapsed to date would likely be better than those using a mutable stopwatch object.

If your purpose is to capture data about how much time is being spent doing various things, I would suggest that you might be best served by a class which builds a list of timing-related events. Such a class may provide a method to generate and add a new timing-related event, which would record a snapshot of its created time and provide a method to indicate its completion. The outer class would also provide a method to retrieve a list of all timing events registered to date.

If the code which creates a new timing event supplies a parameter indicating its purpose, code at the end which examines the list could ascertain whether all events that were initiated have been properly completed, and identify any that had not; it could also identify if any events were contained entirely within others or overlapped others but were not contained within them. Because each event would have its own independent status, failure to close one event need not interfere with any subsequent events or cause any loss or corruption of timing data related to them (as might occur if e.g. a stopwatch had been accidentally left running when it should have been stopped).

While it's certainly possible to have a mutable stopwatch class which uses start and stop methods, if the intention is that each "stop" action be associated with a particular "start" action, having the "start" action return an object which must be "stopped" will not only ensure such association, but it will allow sensible behavior to be achieved even if an action is started and abandoned.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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