2


I am trying to implement a stopwatch similar to that found at Is there a stopwatch in Java?, but as a Thread.

My SecondsCounter class implements Runnable, and invoking its' start method will then create a thread as a local variable.

It would seem as if this thread is conflicting with the first line in the run method:  this.runningTime = System.currentTimeMillis(); ...because the condition while (this.runningTime < this.endTime) is never broken (an infinite loop)...

So is it incorrect to have a class that both implements Runnable and contains its' Thread as well?



Here is the class with the main method:
public class MultithreadingTest {

    public static void main(String[] args) {
        int totalSeconds = 5;
        SecondsPrinter printer = new SecondsPrinter(totalSeconds);
        printer.startPrinting();
    }
} // end of class


...the seconds printer class:

public class SecondsPrinter {

    // composition here:
    private SecondsCounter clock;

    public SecondsPrinter(int totalSeconds) {
        this.clock = new SecondsCounter(totalSeconds);
    }

    public void startPrinting() {
        this.clock.start();
        while (this.clock.isRunning()) {

            // this is incorrectly always printing the maximum seconds value:
            System.out.println(this.clock.getCurrentSecond());
        }
    }
} // end of class


...and the seconds counter class:

public class SecondsCounter implements Runnable {

    private int totalSeconds, currentSecond;
    private long startTime, runningTime, endTime;
    private Thread thread;

    public SecondsCounter(int totalSeconds) {
        this.totalSeconds = totalSeconds;
    }

    public int getCurrentSecond() {
        return this.currentSecond;
    }

    @Override
    public void run() {
        this.runningTime = System.currentTimeMillis();

        // this is an infinite loop, but it shouldn't be:
        while (this.runningTime < this.endTime) {
            this.currentSecond = (int)(this.endTime - this.runningTime) / 1000;
        }

        // this code is never reached:
        this.stop();
    }

    public void start() {
        this.startTime = System.currentTimeMillis();
        this.runningTime = this.startTime;
        this.endTime = this.startTime + (this.totalSeconds * 1000);

        // multithreading here:
        this.thread = new Thread(this);
        this.thread.start();
    }

    public boolean isRunning() {
        return this.thread.isAlive();
    }

    public void stop() {
        this.thread.interrupt();
        this.thread = null;
    }
} // end of class
Community
  • 1
  • 1
Ian Campbell
  • 2,678
  • 10
  • 56
  • 104
  • That is a lot of code. I would consider how you could simplify it to do what you want. – Peter Lawrey Nov 09 '13 at 08:12
  • @PeterLawrey -- Hmm, I could eliminate the `startTime` variable in the `SecondsCounter` class, and possibly pass milliseconds instead of seconds to its' constructor to simplify some of the unit conversion stuff. I could just use `java.util.Timer` instead, but I'm doing it my way just to experiment with threads. Otherwise, I'm not noticing any *separation of concerns* issues or violations of *DRY*.. What might you suggest? – Ian Campbell Nov 09 '13 at 14:35

1 Answers1

4

Actually, that should be an infinite loop. Look at your loop.

while (this.runningTime < this.endTime) {
    this.currentSecond = (int)(this.endTime - this.runningTime) / 1000;
}

The condition says that it will loop while runningTime < endTime. Where is runningTime updated? If you add something like this to the loop, it should work:

public void run() {
    this.runningTime = System.currentTimeMillis();

    // no longer an infinite loop
    while (this.runningTime < this.endTime) {
        this.currentSecond = (int)(this.endTime - this.runningTime) / 1000;
        this.runningTime = System.currentTimeMillis();
    }

    // this code is now reached.
    this.stop();
}

You could even combine it (or remove the variable runningTime altogether):

public void run() {
    while ((this.runningTime = System.currentTimeMillis()) < this.endTime) {
        this.currentSecond = (int)(this.endTime - this.runningTime) / 1000;
    }

    this.stop();
}
Justin
  • 24,288
  • 12
  • 92
  • 142
  • Oh ha I completely missed that, doh! Now it is working correctly, thanks @Quincunx for the help. – Ian Campbell Nov 09 '13 at 06:12
  • Interesting, I didn't know you could both set and then use a value as a while-loop's condition, thanks @Quincunx! This *only* seems to work when the setting is in parentheses, like so: `while( ( this.runningTime = System.currentTimeMillis() ) < this.endTime) {` ...it doesn't compile without the enclosing parentheses, due to operator precedence issues with `<` it seems. – Ian Campbell Nov 09 '13 at 06:26
  • 1
    @IanCampbell That works because the assignment operator also returns the value it assigns. It only works with parentheses because the assignment operators have the lowest [operation precendence](http://docs.oracle.com/javase/tutorial/java/nutsandbolts/operators.html), so without the parentheses, we get `this.runningTime = (System.currentTimeMillis() < this.endTime);`. Good catch on the parentheses. – Justin Nov 09 '13 at 06:30