Inefficient Stopwatch

I have just finished a simple GUI stopwatch, but some of its code looks like it needs replacing. This is the code:

Clock class (extends Thread):

```
static int hr = 0;
static int min = 0;
static int sec = 0;
static double milisec = 0;
static int rotation = 0;
static long l = 0;
static long m = 0;

public void run() {
while (true) {
if (Stopwatch.started()) {
while (Stopwatch.started()) {
if(rotation == 0) {
l = System.currentTimeMillis();
try {
Thread.sleep(5);
} catch (InterruptedException e) {

}
m = System.currentTimeMillis();
plus((int) m - (int) l);
rotation++;
} else if(rotation == 1) {
try {
Thread.sleep(5);
} catch (InterruptedException e) {

}
l = System.currentTimeMillis();
plus((int) l - (int) m);
rotation--;
}
}
}
if (Stopwatch.resets()) {
hr = 0;
min = 0;
sec = 0;
milisec = 0;
}
System.out.print(""); //For some reason this program won't work if this line isn't here
}
}

public static void plus(int i) {
milisec += i;
if (milisec >= 1000) {
milisec -= 1000;
sec++;
}
if (sec >= 60) {
sec -= 60;
min++;
}
if (min >= 60) {
min -= 60;
hr++;
}
}

public static String getHr() {
return hms(hr);
}

public static String getMin() {
return hms(min);
}

public static String getSec() {
return hms(sec);
}

public static String getMilisec() {
return m(milisec);
}

public static String hms(Integer i) {
String s = i.toString();
if(s.length() == 1) {
s = "0" + s;
}
return s;
}

public sta

class Clock extends Thread

In most cases, this is not a good idea -- there's no particular reason for Clock to be its own thread, rather than running in a thread managed by something else (like an ExecutorService). The more common approach would be

class Clock implements Runnable

promising that Clock will implement a run() method, which will allow you to hand it off to a Thread, or an ExecutorService or whatever.

static int hr = 0;
static int min = 0;
static int sec = 0;
static double milisec = 0;
static int rotation = 0;
static long l = 0;
static long m = 0;

Not a good idea -- you should normally prefer member variables to static variables. It will be much easier to test your code, and reason about what is going on, if Clock holds onto its own data (imagine, for a moment, the headache of two different clocks trying to run at the same time -- disaster).

if(rotation == 0) {
                l = System.currentTimeMillis();
                try {
                    Thread.sleep(5);
                } catch (InterruptedException e) {

                }
                m = System.currentTimeMillis();
                plus((int) m - (int) l);
                rotation++;
            } else if(rotation == 1) {
                try {
                    Thread.sleep(5);
                } catch (InterruptedException e) {

                }
                l = System.currentTimeMillis();
                plus((int) l - (int) m);
                rotation--;
            }

Here's one of the problems with shared variables -- it makes it very hard to come back later and understand the context of what's going on. Your else block modifies l, without modifying m. Is that deliberate? It might be, it might not be. Better variable names here would make clearer the intent of the code.

For instance, I'd like to suggest locally scoped variables

long before = System.currentTimeMillis();
                try {
                    Thread.sleep(5);
                } catch (InterruptedException e) {

                }
                long after = System.currentTimeMillis();
                plus(after - before);

But you could be using l and m anywhere, so maybe this works, maybe it doesn't. Encapsulation is an important clue for people trying to understand your code.

This particular construction looks suspicious:

if(rotation == 0) {
                ...;
                rotation++;
            } else if(rotation == 1) {
                ...;
                rotation--;
            }

0 and 1 are not very good for expressing intent - defining a constant that explains what each of these numbers means would be a big help for those reading the code. Also, this looks like you are trying to implement a state machine - "if in this state, verb this way, then transition to that state" - and if that's what it's supposed to look like, then you should actually implement States and transitions so that it is obvious.

try {
            start.setText("Start");
        } catch (Exception e) {

        }

The empty exception block is a bad sign - the catch block that you don't know how JButton works. A checked exception being thrown is an indication that there is a legitimate error condition that your code is supposed to recover from. Dropping it on the floor without leaving any evidence behind is very poor form. Even if you are absolutely certain that the Exception should not impact the behavior of your application in any way, at a minimum there should be a comment explaining why this is the case.

try {
                    Thread.sleep(5);
                } catch (InterruptedException e) {

                }

A very bad sign - this indicates that you don't understand how cancellation and shutdown work. InterruptedException are an important part of communication, and should not be dismissed without comment. (You might reasonably dismiss InterruptedExceptions with comment - not all operations should be interruptable -- but you will usually reset the interrupted flag in case your caller cares about interruption). In this case, where you are the Thread, and never delegate anything, the interrupted flag isn't so important.

System.out.print("");    //For some reason this program won't work if this line isn't here

Not surprising. It probably doesn't work with the line there either.

My guess is that the print call is introducing a memory barrier, that flushes the caches, but I won't swear to it. The very hand waving explanation being that the two threads you have created have no code in them indicating that any other thread needs to share the data, so the two threads are each happy spinning in tight loops, updating values in their local registers, and never checking to see if the common values of those variables have changed. However, the System.out.print call does have in it some code that knows about shared data, and your code appears to work w

StackExchange Code Review Q#57252, answer score: 7