An alarm application in Java

I have programmed this alarm application in Java using JavaFX for the GUI. I'd like these questions covered in the review:

• Is my code efficient? Can it be shortened?

• Are there any flaws in my code?

• Do I violate any OOP principles? If so, how, where and how do I rectify them?

App UI:

Alarm.java:

/*
 * Program developed by Hassan Althaf.
 * Copyright © 2015, Hassan Althaf.
 * Website: http://hassanalthaf.com
 */
package com.HassanAlthaf;

import javafx.application.Application;
import javafx.fxml.FXMLLoader;
import javafx.scene.Parent;
import javafx.scene.Scene;
import javafx.stage.Stage;

public class Alarm extends Application {

    @Override
    public void start(Stage stage) throws Exception {
        Parent root = FXMLLoader.load(getClass().getResource("MainWindow.fxml"));
        Scene scene = new Scene(root);

        stage.setScene(scene);
        stage.setResizable(false);
        stage.setTitle("Alarm Application - Hassan Althaf");

        stage.show();
    }

    public static void main(String[] args) {
        launch(args);
    }
}

MainView.java:

```
/*
* Program developed by Hassan Althaf.
* Copyright © 2015, Hassan Althaf.
* Website: http://hassanalthaf.com
*/
package com.HassanAlthaf;

import java.net.URL;
import java.util.ResourceBundle;
import java.util.Timer;
import java.util.TimerTask;
import javafx.event.ActionEvent;
import javafx.fxml.FXML;
import javafx.fxml.Initializable;
import javafx.scene.control.Alert;
import javafx.scene.control.Alert.AlertType;
import javafx.scene.control.TextField;

/**
*
* @author hassan
*/
public class MainView implements Initializable {

@FXML
private TextField hoursField;

@FXML
private TextField minutesField;

@FXML
private TextField secondsField;

private final AlarmController alarmController;
private Timer timer;
private boolean timerOn = false;
private boolean alarmPlayed = false;
private boolean timerValidation = true;

@FXML
private

It seems good overall.

-
I'm not familiar with JavaFX threading, but it seems that you at least have a threading bug for the shared variables which you modify within your TimerTask.run(). The flags alarmPlayed and timerOn should be declared AtomicBoolean. I checked the doc of MediaPlayer to see if it can be started and stopped from different threads. I did not find an answer, but I would guess it does.

-
I would invert the two lines:

this.timer.schedule(task, 0, 1000);
 this.timerOn = true;

but more generally, I think I would get rid of this.timerOn and instead just set this.timer = null to indicate there is no timer.

-
It not clear why you are waking up every second to check if the time is done instead of just setting the timer once for the final time. The later would simplify your code a lot. My guess is that maybe you were planning to add some animation at each second in the future.

-
private final int ALARM_TIME = totalTime is not needed. You can use totalTime directly within TimerTask.run(). You might have to declare it final.

-
You sometimes use C bracket placement.

Answers to Questions in the Commments:

Multi-threading (also known as concurrency) is a difficult and subtle topic. The best reference is Java Concurrency In Practice; you can also take a look at the tutorial. The problem is that modern cpus don't have a single copy of each variable in memory, but have many copies since each cpu also has some cache. Thread-safe code means that you make sure that when a thread modifies a variable, the other threads see that change. You don't want all variables to be shared between threads, only those that do need to be shared, since keeping variables up-to-date between cores has some cost since the values in the various core caches must be synchronized.

In your case, you have two different threads: 1) the thread that runs the JavaFX UI and 2) the timer thread which runs TimerTask.run(). The two variables alarmPlayed and timerOn are used by both threads. What can happen is that your code in the timer can change those variables, but that change might never be seen by the code in the JavaFX UI thread.

Where things get even more tricky is that although your program might have a bug formally, it can happen in practice that for the particular cpu you are using, this bug might never arise. Where things get even trickier, is that it is even possible that the timer has some inner synchronization that makes it unnecessary to add any other explicit synchronization controls. One would have to understand the Java Memory Model to tell if it's the case or not, but I don't think anyone fully understands it except the guys who wrote Java.

AtomicBoolean, or volatile boolean, means that whenever such a variable is modified by one thread, all threads immediately see the updated value (the caches between all cpu cores are synchronized).

For your question about the timer not waking every single second, there are also methods in Timer which instead of calling a repeating task, just call the task a single time. See this version of Timer.schedule which takes as argument a task and the number of milliseconds after which the task will be executed, a single time.

public class A { // Java bracket placement
     public void method()
     {  // C bracket placement (should be at the end of the above line)
        ...
     }
}

StackExchange Code Review Q#106446, answer score: 5