Bouncing square in a box

Question

I wrote my first animation in Java FX and would like a code review.

I created StackPane with Rectangle. Rectangle starts to move inside StackPane to right-bottom direction and changes direction after hitting the StackPane bound.

Please review the code that moves Rectangle. Is it a correct implementation of a Java FX animation?

//... 
new RectangleMover(pane, shape); 
// ...

RectangleMover.java

public class RectangleMover extends Timer
{
 private final StackPane pane;
 private final Rectangle shape;
 private boolean moveRight = true;
 private boolean moveBottom = true;
 public RectangleMover(StackPane pane, Rectangle shape)
 {
 super(true);
 this.pane = pane;
 this.shape = shape;
 this.scheduleAtFixedRate(new Task(), 0, 10);
 }
 private static int boolToInt(boolean bool)
 {
 return bool ? 1 : -1;
 }
 private final class Task extends TimerTask
 {
 @Override
 public void run()
 {
 shape.setLayoutX(shape.getLayoutX() + boolToInt(moveRight));
 shape.setLayoutY(shape.getLayoutY() + boolToInt(moveBottom));
 if (shape.getLayoutX() >= pane.getWidth() - shape.getWidth())
 {
 moveRight = false; 
 }
 if (shape.getLayoutY() >= pane.getHeight() - shape.getHeight())
 {
 moveBottom = false;
 }
 if (shape.getLayoutX() <= 0)
 {
 moveRight = true;
 }
 if (shape.getLayoutY() <= 0)
 {
 moveBottom = true;
 }
 }
 }
} 

JFXTester.java (main class)

import javafx.application.Application;
import javafx.event.ActionEvent;
import javafx.event.EventHandler;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.layout.StackPane;
import javafx.scene.paint.Color;
import javafx.scene.shape.Rectangle;
import javafx.stage.Stage;
public class JFXTester extends Application
{
 StackPane root;
 @Override
 public void start(Stage primaryStage)
 {
 root = new StackPane();
 root.getChildren().add(createStartButton());
 Scene scene = new Scene(root, 300, 250);
 primaryStage.setTitle("Hello World!");
 primaryStage.setScene(scene);
 primaryStage.show();
 }
 private Rectangle createRectangle()
 {
 Rectangle r = new Rectangle();
 r.setX(0);
 r.setY(0);
 r.setWidth(30);
 r.setHeight(30);
 r.setFill(new Color(0.8, 0.7, 0.6, 0.5));
 return r;
 }
 private Button createStartButton()
 {
 final Button btn = new Button();
 btn.setText("START!");
 btn.setOnAction(new EventHandler<ActionEvent>()
 {
 @Override
 public void handle(ActionEvent event)
 {
 btn.setVisible(false);
 Rectangle r = createRectangle();
 root.getChildren().add(r);
 new RectangleMover(root, r);
 }
 });
 return btn;
 }
 public static void main(String[] args)
 {
 launch(args);
 }
} 

How to run application (java 1.7.0_51-b13):

D:\.temporary\java>dir /a-d /b /s
D:\.temporary\java\jfxtester\JFXTester.java
D:\.temporary\java\jfxtester\RectangleMover.java
D:\.temporary\java>%JAVA_HOME%\bin\javac -cp .;%JRE_HOME_7%\lib\jfxrt.jar ./jfxtester/JFXTester.java 
D:\.temporary\java>%JRE_HOME_7%\bin\java -cp .;%JRE_HOME_7%\lib\jfxrt.jar jfxtester.JFXTester

• Simon Forsberg: Welcome to Code Review! Have you tested your code? Code Review is about making working things cleaner, but a simple test fails to get your code working properly. Exception in thread "Timer-0" java.lang.IllegalStateException: Not on FX application thread; currentThread = Timer-0. I would recommend posting this on StackOverflow, with this specific error message.
• Ilya: Yes, the code is working as expected
• Simon Forsberg: Please provide if possible sufficient code for it to work.
• Ilya: @SimonAndréForsberg I have added main class. Java 7 is used
• Simon Forsberg: I'm sorry but I don't believe this code is working correctly (because of Exception in thread "Timer-0" java.lang.IllegalStateException: Not on FX application thread; currentThread = Timer-0). You ask "Is it a correct implementation of a Java FX animation?" and I don't think this is working as expected as you say. I would love to help you clean up some things in the code, but it is better if you fix the problem first. If you search for "Not on FX application thread" you should find a solution. Please come back and update your code here once you have fixed that problem.

Answer 1

No, this is not the correct way to do it. I would recommend that you read JavaFX Transitions documentation.

I personally could not get your code to work properly (Using Java 8, but that should not matter as Java is always backwards compatible). The rectangle showed on the screen, but it did not move at all.

I got this exception:

Exception in thread "Timer-0" java.lang.IllegalStateException: Not on FX application thread; currentThread = Timer-0
at com.sun.javafx.tk.Toolkit.checkFxUserThread(Unknown Source)
at com.sun.javafx.tk.quantum.QuantumToolkit.checkFxUserThread(Unknown Source)
at javafx.scene.Scene.addToDirtyList(Unknown Source)
at javafx.scene.Node.addToSceneDirtyList(Unknown Source)
at javafx.scene.Node.impl_markDirty(Unknown Source)
at javafx.scene.shape.Shape.impl_markDirty(Unknown Source)
at javafx.scene.Node.impl_transformsChanged(Unknown Source)
at javafx.scene.Node13ドル.invalidated(Unknown Source)
at javafx.beans.property.DoublePropertyBase.markInvalid(Unknown Source)
at javafx.beans.property.DoublePropertyBase.set(Unknown Source)
at javafx.scene.Node.setLayoutX(Unknown Source)

One I fixed that problem to make it run on the JavaFX thread, the rectangle still did not move an (削除) inch (削除ここまで) pixel.

About the cleaniness of your code:

public class RectangleMover extends Timer

extends Timer is a very bad smell here. There's absolutely no reason to extend Timer. Use a timer as a field inside your class instead. Prefer composition over inheritance.

Speaking of Timer, you could say that it is more or less deprecated, it is often better to use a ScheduledExecutorService. However, if you do the animation properly, you will not need a Timer or ExecutorService at all.

I do like how you're using your boolToInt method (just a shame that I can't the results of it does not seem to be working). I also like how you have separated your methods overall.

I just don't like that your code does not seem to be working for me...

• Ilya: You are right, inheritance with Timer is redundant here. Also advice about ExecutorService is very helpful for me. Thank you