JavaFX Exercise Application

Question

This is a pre version of my program. This program is about solving exercises. Because this is one of my first graphical applications, I want to know, what can be improved. To this point I usually created command line programs.

You can download the executable here

Main.java

import javafx.application.Application;
import javafx.stage.Stage;
import javafx.scene.Scene;
import javafx.scene.layout.VBox;
import javafx.scene.layout.HBox;
import javafx.scene.control.Label;
import javafx.scene.control.Button;
import javafx.scene.control.TextField;
import javafx.scene.control.Alert;
import javafx.scene.control.Alert.AlertType;

/* This class represents a program in which the player has to solve 15
 * exercises.
 * After he clicks the check button, he can see a summary.
 */
public class Main extends Application {
    @Override
    public void start(Stage primaryStage) {
        int numberOfExercises = 15;

        // create exercises
        Exercise[] exercises = new Exercise[numberOfExercises];
        for (int i = 0; i < numberOfExercises; i++) {
            exercises[i] = Exercise.getRandomExercise();
        }

        // store text fields for later use in an array
        TextField textFields[] = new TextField[numberOfExercises];

        // display exercises graphically
        VBox content = new VBox(numberOfExercises + 1);
        for (int i = 0; i < numberOfExercises; i++) {
            Label exerciseDescription = new Label(exercises[i]
              .toString());
            exerciseDescription.setPrefWidth(100);
            textFields[i] = new TextField();
            textFields[i].setPrefWidth(70);
            HBox exerciseBox = new HBox(2);
            exerciseBox.getChildren().addAll(exerciseDescription, 
              textFields[i]);
            content.getChildren().add(exerciseBox);
        }

        Button button = new Button("Check results");
        button.setOnAction(event -> {
            // check if exercises are solved correctly
            int correctExercises = 0;
            for (int i = 0; i < exercises.length; i++) {
                int guess = 0;
                try {
                    guess = Integer.parseInt(textFields[i].getText());
                } catch (Exception e) {
                    e.printStackTrace();
                }
                if (exercises[i].compare(guess)) {
                    correctExercises++;
                }
            }

            // display a message
            Alert alert = new Alert(AlertType.INFORMATION);
            alert.setTitle("Congratulations!");
            alert.setHeaderText(null);
            String message = "You solved " + correctExercises +
              " of " + numberOfExercises + " exercises correctly!";
            alert.setContentText(message);
            alert.showAndWait();

        });
        content.getChildren().add(button);

        primaryStage.setScene(new Scene(content));
        primaryStage.show();
    }

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

Exercise.java

import java.util.Random;

public class Exercise {
    private int number1;
    private int number2;
    private int solution;
    private String operationSymbol;

    private Exercise(int number1, int number2, int solution, 
      String operationSymbol) {
        this.number1 = number1;
        this.number2 = number2;
        this.solution = solution;
        this.operationSymbol = operationSymbol;
    }

    public static Exercise getRandomExercise() {
        Random random = new Random();
        int choice = random.nextInt(4);

        int number1;
        int number2;
        int solution;
        String operationSymbol;
        if (choice == 0) {
            number1 = random.nextInt(26);
            number2 = random.nextInt(26);
            solution = number1 + number2;
            operationSymbol = "+";
        } else if (choice == 1) {
            number1 = random.nextInt(16) + 10;
            number2 = random.nextInt(26);
            solution = number1 - number2;
            operationSymbol = "-";          
        } else if (choice == 2) {
            number1 = random.nextInt(15) + 1;
            number2 = random.nextInt(15) + 1;
            solution = number1 * number2;
            operationSymbol = "*";          
        } else {
            number1 = random.nextInt(151);
            number2 = random.nextInt(15) + 1;
            solution = number1 / number2;
            operationSymbol = "/";          
        }
        return new Exercise(number1, number2, solution, 
          operationSymbol);
    }

    public boolean compare(int guess) {
        return guess == solution;
    }

    @Override 
    public String toString() {
        return number1 + " " + operationSymbol + " " + number2 + " = ";
    }
}

Answer 1

Separation of concerns

The start method of an application should be responsible for initializing application-wide basic functionality and trigger the entry point of the application. This means it should deal with Dependency Injection, Application Resource Reservation and similar "global" concerns. Setting up the GUI is not the primary concern of that method.

Instead you'll want it to be something like:

@Override
public void start(Stage primaryStage) {
   MainView view = new MainView();
   primaryStage.setScene(view.getScene());
   primaryStage.show();
}

This allows you to encapsulate the View into it's own proper class only responsible for the view.

Setting up the View

Your View setup intermingles domain logic (what's an Excercise?) with View logic (How are components laid out?). In addition there's a handful of simplifications to be had there.

• numberOfExercises is a constant, treat it as one.
• The HBox containing all the information about a single exercise could be extracted into a separate component that takes an Exercise as constructor argument and correctly deals with setting up description, solution and encapsulates the correctness checking.
• Your setOnAction for the button seems rather complex. It could benefit a lot from being extracted into a separate method.

Formatting

You seem to be following the old school of line-length considerations. Lines in this day and age can (and should) be longer than 80 characters. For one you're using 4 spaces for indentation, and secondly nowadays even the smaller screens are able to support 100-120 characters in width with relative ease.

Forcing the code to adhere to outdated styling regulations makes it hard to read that code.

ToString is not a user-level representation

Your code is currently using toString to display a domain object to the user. That's not how this should work. A user representation is usually one that contains less information than the whole object. In general you want to be able to toString an object for logging purposes, incorporating all information available in the object.

Note also that polymorphism makes it nearly trivial to make the code you've written display nonsensical data:

public class SpecialExercise extends Exercise {
    public SpecialExercise() {
        super(0, 0, 0, "");
    }
    @Override
    public String toString() {
        // basically Object.toString()`
        return super.super.toString();
    }
}