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 when the print call forces local data to refresh.
No promises, but maybe.
Welcome to the horror show that is multi threaded programming.
The good news, is that your data is relatively simple, in the sense that each shared piece of data has only one thread that writes it. For example, the Running/NotRunning state of the stopwatch needs to be visible to the Stopwatch and the Clock, but only the Stopwatch needs to change it (which happens when the button event handlers are called in the UI thread). Similarly, both pieces need to read the current clock time, but only the Clock thread needs to modify it.
You should refactor the code so that these bits of data are
- separate from the classes that share them
- separate from each other
For example:
// This bit is used by all the threads
interface StopwatchState {
State getCurrentState();
}
// This bit is used only by the thread that changes the state
interface StopwatchController {
void stop();
void start();
// not my choice, but similar to the implementation in your example;
void toggle();
}
class MultiThreadedStopwatchController implements StopwatchState, StopwatchController {
private volatile State state;
private static final EnumMap<State,State> transitions = ...;
MultiThreadedStopwatchController () {
state = State.STOPPED;
}
State getCurrentState () {
return state;
}
void stop() {
state = State.STOPPED;
}
void start() {
state = State.STARTED;
}
void toggle() {
state = transitions.get(state);
}
}
...
class Clock {
private final StopwatchState stopwatch;
Clock (StopwatchState stopwatch) {
this.stopwatch = stopwatch;
}
void run() {
if (State.STARTED.equals(stopwatch.getCurrentState())) {
....
}
}
}
...
class Stopwatch {
private final StopwatchController controller;
Stopwatch(StopwatchController controller) {
this.controller = controller;
...
start = new JButton("Start");
start.addMouseListener(new MouseAdapter() {
public void mouseReleased(MouseEvent e) {
controller.toggle();
}});
MultiThreadedStopwatchController stopwatchController = new (...);
Clock clock = new Clock(stopwatchController);
Stopwatch stopwatch = new Stopwatch(stopwatchController);
And then you can start to worry whether Clock should know that there's a state machine under the covers, of if instead the interface should look more like....
void run() {
if (stopwatch.isRunning()) {
....
}
}
(Which it probably should - the interface should specify what, not how).
StateMachines
There are degrees of how complicated to get with state machines. What happens most often is that programmers don't notice that they are implementing one, and the logic gets scattered all over.
The stop watch here is a pretty simple example of a state machine -- we've got a mouseReleased event, that is supposed to change a stopped watch to a running watch, or a running watch to a stopped watch. Describing that more generally, the watch is in one of two states (running, stopped); and the mouseRelease event should change which state the watch is in.
A common pattern, on recognizing that there is a state machine in place, is to define an Enum that describes the states.
public enum State {
STARTED, STOPPED;
}
And then an object to hold a particular instance of the machine that is transitioning between states
public class FSM {
volatile State currentState;
FSM(State initialState) {
this.currentState = initialState;
}
...
}
We use the volatile keyword here because we know that FSM is going to be read by a thread other than the one that writes to it, and we need that value to be visible across all the threads. At a very hand-waving level, the volatile keyword tells the JVM that when this value is written, the value written needs to be pushed all the way out to share memory right away.
Part of describing the state machine is defining which state changes are legal. It would be straighforward to write this out, long hand:
if (State.STARTED.equals(currentState)) {
currentState = State.STOPPED;
} else {
currentState = State.STARTED;
}
Which is fine... when you are dealing with only two states. But as you discover more states (STOPPED, STARTED, RESET...), you start needing to write out more transitions by hand.
But, if we look carefully, we're really just doing a lookup here, and we can implement a lookup with a Map
// Note: this map is shared by all instances of this kind of state machine
static final Map<State, State> stateTransitions = new HashMap();
static {
// when we initialize the class, we load the transition map
stateTransitions.put(State.STARTED, State.STOPPED);
stateTransitions.put(State.STOPPED, State.STARTED);
}
// here, we're changing the state of a specific instance
public void toggle() {
currentState = stateTransitions.get(currentState);
}
Now, because our State tokens are implemented as an enum (meaning that each of the States are really singletons), we can use an EnumMap - which is a Map that is optimized for the case where all of the keys are required to be from a specific enumeration.
Having written the code this way, we can change our two mode button to a three mode button just by adding a new value into the enumeration, and updating the transition table.
static {
// when we initialize the class, we load the transition map
stateTransitions.put(State.STARTED, State.STOPPED);
stateTransitions.put(State.STOPPED, State.RESET);
stateTransitions.put(State.RESET, State.STARTED);
}
And presto - everything works.
Using a single Map works here because this is a toy problem, each state always goes to the "next" state.
In a slightly more complicated problem, you can have more than one kind of transition - a STARTED watch can be STOPPED or PAUSED.... The trivial Map is no longer appropriate - we need to map State + Trigger = new State.
// Note: this map is shared by all instances of this kind of state machine
static final EnumMap<State, EnumMap<Trigger,State>> stateTransitions = ...;
It's really this map that defines your state machine - you could use the same States and the same Triggers, organized in a different way, to produce a different class of machines.
static {
// when we initialize the FSM, we load the transition map
// first, we create empty Trigger maps for each known state
for (State s : State.values()) {
stateTransitions.put(s, new EnumMap<Trigger,State>());
}
// now fill the trigger maps with the supported transitions
stateTransitions.get(State.STARTED).put(Trigger.TOGGLE, State.STOPPED);
stateTransitions.get(State.STOPPED).put(Trigger.TOGGLE, State.STARTED);
// here, we add pause support.
stateTransitions.get(State.STARTED).put(Trigger.PAUSE, State.PAUSED);
stateTransitions.get(State.PAUSED).put(Trigger.PAUSE, State.STARTED);
}
Stateless4j puts a reasonable fluent interface in front of the state machine creation idioms. Using that library, your state machine creation might look like...
StateMachine<State, Trigger> stopwatch = new StateMachine<State, Trigger> ();
stopwatch.configure(State.STARTED)
.permit(Trigger.TOGGLE, State.STOPPED)
.permit(Trigger.PAUSE, State.PAUSED);
stopwatch.configure(State.STOPPED)
.permit(Trigger.TOGGLE, State.STARTED);
stopwatch.configure(State.PAUSED)
.permit(Trigger.TOGGLE, State.STARTED);
