An alarm application in Java

Question

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 void startAlarm(ActionEvent event) {
        if(!timerOn) {
            this.alarmPlayed = false;
            this.timerValidation = true;

            String hoursString = this.hoursField.getText();
            String minutesString = this.minutesField.getText();
            String secondsString = this.secondsField.getText();

            if(hoursString.equals("")) {
                hoursString = "0";
            }

            if(minutesString.equals("")) {
                minutesString = "0";
            }

            if(secondsString.equals("")) {
                secondsString = "0";
            }

            int hours = 0;
            int minutes = 0;
            int seconds = 0;

            try {
                hours = Integer.parseInt(hoursString);
                minutes = Integer.parseInt(minutesString);
                seconds = Integer.parseInt(secondsString);
            } catch(Exception ex) {
                this.timerValidation = false;
                this.showWarning("Please enter a number only for the times!", "Invalid input received");
            }

            int totalTime = seconds + (minutes * 60) + (hours * 3600);

            if(totalTime == 0) {
                this.timerValidation = false;
                this.showWarning("Please set a time for the alarm before starting!", "Time not found!");
            }

            if(this.timerValidation) {
                TimerTask task = new TimerTask() {
                    private final int ALARM_TIME = totalTime;
                    private int secondCounter = 0;

                    @Override
                    public void run()
                    {
                        if(secondCounter < ALARM_TIME) {
                            secondCounter++;
                        } else {
                            if(timerOn) {
                                alarmController.playAlarm();
                                alarmPlayed = true;
                            }
                            timerOn = false;
                            cancel();
                        }
                    }
                };

                this.timer = new Timer();
                this.timer.schedule(task, 0, 1000);

                this.timerOn = true;
            }
        } else {
            this.showWarning("An alarm is already running at the moment! If you wish to override the current alarm, please stop it first!", "Cannot override current alarm.");
        }
    }

    public void showWarning(String message, String title) {
        Alert alert = new Alert(AlertType.WARNING);
        alert.setTitle(title);
        alert.setHeaderText(null);
        alert.setContentText(message);
        alert.showAndWait();
    }

    @FXML
    private void stopAlarm(ActionEvent event) {
        if(this.timerOn) {
            this.timer.cancel();
            this.timer.purge();
            this.timerOn = false;
        }
        if(this.alarmPlayed) {
            this.alarmController.stopAlarm();
            this.alarmPlayed = false;
        }
    }

    public MainView() {
        this.alarmController = new AlarmController();
    }

    @Override
    public void initialize(URL url, ResourceBundle rb) {

    }    
}

MainWindow.fxml:

<?xml version="1.0" encoding="UTF-8"?>

<?import javafx.scene.text.*?>
<?import java.lang.*?>
<?import java.util.*?>
<?import javafx.scene.*?>
<?import javafx.scene.control.*?>
<?import javafx.scene.layout.*?>

<AnchorPane id="AnchorPane" maxHeight="300.0" maxWidth="500.0" minHeight="300.0" minWidth="500.0" prefHeight="300.0" prefWidth="500.0" xmlns="http://javafx.com/javafx/8" xmlns:fx="http://javafx.com/fxml/1" fx:controller="com.HassanAlthaf.MainView">
   <children>
      <Label layoutX="171.0" layoutY="30.0" text="Alarm Application" textFill="#404040">
         <font>
            <Font name="Lato Regular" size="20.0" />
         </font>
      </Label>
      <Label layoutX="150.0" layoutY="68.0" text="Set the timer for your alarm below" />
      <TextField fx:id="hoursField" layoutX="193.0" layoutY="108.0" />
      <Label layoutX="150.0" layoutY="113.0" text="Hours: " />
      <Label layoutX="138.0" layoutY="150.0" text="Minutes: " />
      <TextField fx:id="minutesField" layoutX="193.0" layoutY="145.0" />
      <TextField fx:id="secondsField" layoutX="193.0" layoutY="182.0" />
      <Label layoutX="133.0" layoutY="187.0" text="Seconds: " />
      <Button layoutX="251.0" layoutY="222.0" mnemonicParsing="false" onAction="#startAlarm" text="Start" />
      <Button layoutX="309.0" layoutY="222.0" mnemonicParsing="false" onAction="#stopAlarm" text="Stop" />
   </children>
</AnchorPane>

AlarmController.java:

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

import javafx.scene.media.Media;
import javafx.scene.media.MediaPlayer;

/**
 *
 * @author hassan
 */
public class AlarmController {
    private MediaPlayer mediaPlayer;

    public void playAlarm()
    {
        Media media = new Media(getClass().getResource("alarm.mp3").toString());
        this.mediaPlayer = new MediaPlayer(media);
        this.mediaPlayer.play();
    }

    public void stopAlarm()
    {
        this.mediaPlayer.stop();
    }
}

Answer 1

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)
        ...
     }
}

Answer 2

Here are some things that can be improved:

private boolean timerValidation = true;

This field is only used inside startAlarm, and its value is relevant only during the execution of that method. Therefore, it is actually a temporary variable of startAlarm and should be declared inside it: (I also removed the unused initial value)

@FXML
private void startAlarm(ActionEvent event) {
    boolean timerValidation;

---

@FXML
private void startAlarm(ActionEvent event) {
    boolean timerValidation;

    if(!timerOn) {
        /* Main code of the method */
        /* ... */
    } else {
        this.showWarning("An alarm is already running at the moment! If you wish to override the current alarm, please stop it first!", "Cannot override current alarm.");
    }
}

The main code of the method is needlessly indented because of the conditional. It is better to test the short branch first (the case when timerOn is true, which has only 1 line) and exit early (return immediately from the method):

private void startAlarm(ActionEvent event) {
    boolean timerValidation;

    if(timerOn) {
        this.showWarning("An alarm is already running at the moment! If you wish to override the current alarm, please stop it first!", "Cannot override current alarm.");
        return;
    }

    /* Main code of the method */
    /* ... */
}

This eliminates the extra indentation.

---

String hoursString = this.hoursField.getText();
String minutesString = this.minutesField.getText();
String secondsString = this.secondsField.getText();

if(hoursString.equals("")) {
    hoursString = "0";
}

if(minutesString.equals("")) {
    minutesString = "0";
}

if(secondsString.equals("")) {
    secondsString = "0";
}

int hours = 0;
int minutes = 0;
int seconds = 0;

try {
    hours = Integer.parseInt(hoursString);
    minutes = Integer.parseInt(minutesString);
    seconds = Integer.parseInt(secondsString);
} catch(Exception ex) {
    timerValidation = false;
    this.showWarning("Please enter a number only for the times!", "Invalid input received");
}

The same logic is repeated for each of the field. You can extract it to a new method to eliminate the duplication:

private int readFieldAsNumber(TextField field) {
    /* TODO: return 0 if field is empty, otherwise parse input as integer */ 
    /* This will throw an exception if the input is not a number */
}

So you can use it like this:

int hours = 0, minutes = 0, seconds = 0;

try {
    hours = this.readFieldAsNumber(hoursField);
    minutes = this.readFieldAsNumber(minutesField);
    seconds = this.readFieldAsNumber(secondsField);
} catch(NumberFormatException ex) {
    timerValidation = false;
    this.showWarning("Please enter a number only for the times!", "Invalid input received");
}

Notice I changed the exception type that is caught to NumberFormatException. The code inside the catch can't handle any other exception.

---

The timerValidation variable is used as a flag that signifies the method should return. The code can be made simpler by getting rid of that variable and simply returning immediately when needed. For example:

} catch(Exception ex) {
    this.showWarning("Please enter a number only for the times!", "Invalid input received");
    return;
}

Or:

if(totalTime == 0) {
    this.showWarning("Please set a time for the alarm before starting!", "Time not found!");
    return
}

This way you don't need to surround the code that initializes the timer with the if(this.timerValidation) condition. You know that the time is valid (otherwise the function would have returned earlier). So you can eliminate the extra indentation.

---

Rename AlarmController to AlarmPlayer so it's easy to know what it does. I also recommend renaming its methods to play and stop because the name class already tells us it's controlling the alarm.

---

Good luck!

EDIT: As @toto2 suggested, you can have the timer fire off just once (instead of every second) and set its delay to the requested duration. See Timer#schedule with 2 arguments

Answer 3

I found some problems with the OOP design:

The MainView class has the following field, which keeps track of whether the alarm is playing or not:

private boolean alarmPlayed = false;

However this is not a part of its core responsibility, which is to control the alarm based on the user input. The MainView is responsible for knowing whether the alarm is playing, but not for computing this boolean value (the "computing" here is setting the variable to true and false when needed). I would say this violates the Single Responsibility Principle.

Note that every time you call a method of AlarmController you're updating alarmPlayed. Here:

alarmController.playAlarm();
alarmPlayed = true;

And here:

this.alarmController.stopAlarm();
this.alarmPlayed = false;

This suggests that MainView is managing AlarmController's state, instead of AlarmController managing its own state. The alarmPlayed field really belongs to AlarmController.

The solution is to move the alarmPlayed field from MainView to the AlarmController. Then make AlarmController update this field whenever its methods are called, and add a public isAlarmPlaying method to AlarmController to expose this field:

public class AlarmController {
    private MediaPlayer mediaPlayer;
    private boolean alarmPlayed = false;

    public void playAlarm() {
        Media media = new Media(getClass().getResource("alarm.mp3").toString());
        this.mediaPlayer = new MediaPlayer(media);
        this.mediaPlayer.play();
        alarmPlayed = true;
    }

    public void stopAlarm() {
        this.mediaPlayer.stop();
        this.alarmPlayed = false;
    }

    public boolean isAlarmPlaying() {
        return this.alarmPlayer;
    }
}

The benefit is that MainView no longer needs to manage this field. It is now managed by the class that is responsible for it, AlarmController. This makes MainView simpler because it now doesn't handle this unrelated task.

---

Similarly, the timerOn field is tracking timer's state.

private boolean timerOn = false;

It is always updated after a call to timer. Here:

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

And here:

this.timer.cancel();
this.timer.purge();
this.timerOn = false;

This one is trickier, because you're using Java's Timer class which doesn't have a method that tells you whether a task is currently scheduled. In this case, you'll have to wrap Java's Timer class inside your own class to get the interface you want:

public class MyTimer {
    private Timer timer;
    private boolean timerOn = false;

    public void scheduleOnEverySecond(TimerTask task) {
        this.timer = new Timer();
        this.timer.schedule(task, 0, 1000);

        this.timerOn = true;
    }

    public void cancel() {
        this.timer.cancel();
        this.timer.purge();
        this.timerOn = false;
    }

    public boolean isTimerRunning() {
        return this.timerOn;
    }
}

MyTimer is a proxy to Java's Timer.

Notice that MyTimer provides exactly the interface you need for your application: scheduleOnEverySecond, cancel, and isTimerRunning. Now MainView can be simplified by getting rid of timerOn and using MyTimer instead of Timer. The code in MainView that controls the timer becomes simply this:

this.myTimer.scheduleOnEverySecond(task);

and this:

this.myTimer.cancel();

This makes MainView much simpler and easier to read because it now doesn't handle the complexity of using Java's Timer (whose interface is too generic for your application). Instead, MainView's is focused on doing its sole responsibility: getting the user's input and using the timer and AlarmController to schedule an alarm.

(MyTimer should be renamed, but I can't think of a good name right now)

---

To summarize, if a class requires you to keep extra state to use it, then it fails to fulfill its responsibility. A class should be responsible for managing the state required to use it.