5
\$\begingroup\$

The assignment description for this code is this:

Each time the user clicks the Circles button, randomly colored circles flow into the display and continuously change their shapes.

Here is a screenshot of my program running:

enter image description here

This is my first time seriously exploring the aspects of Java 8 development. Therefore, I would like reviews to be tailored as such, since I would like to know the best way for using it in the future.


package java8;

import java.util.stream.Stream;
import javafx.animation.ScaleTransition;
import javafx.animation.TranslateTransition;
import javafx.application.Application;
import javafx.geometry.Pos;
import javafx.scene.Node;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.layout.Pane;
import javafx.scene.layout.VBox;
import javafx.scene.paint.Color;
import javafx.scene.paint.Paint;
import javafx.scene.shape.Circle;
import javafx.stage.Stage;
import javafx.util.Duration;

/**
 * A lab exercise to introduce Java 8 streams and JavaFX
 * @author syb0rg
 */
public class Java8 extends Application {

    public static final int ROWS = 4;
    public static final int COLS = 5;
    public static final int CELL_SIZE = 100;
    public static final int RADIUS = 25;

    /**
     * This method kicks off the initialization process.
     * @param primaryStage the main frame
     */
    @Override
    public void start(Stage primaryStage) {
        root = new VBox();
        canvas = new Pane();
        starter = new Button("Circles");

        configure();
        addButtonHandler();

        root.getChildren().addAll(canvas, starter);

        primaryStage.setTitle("Java 8 Lab Exercise");
        primaryStage.setScene(new Scene(root));
        primaryStage.show();
    }

    /**
     * This method sets the alignment of the root container
     * and the size of the canvas.
     */
    private void configure() {
        root.setAlignment(Pos.CENTER);
        canvas.setPrefSize(COLS*CELL_SIZE, ROWS*CELL_SIZE);
    }

    /**
     * This method adds the handler to the button that gives
     * this application its behavior.
     */
    private void addButtonHandler() {
        starter.setOnAction(e -> {
            circles();
        });
    }

    /**
     * This method creates a column of circles using Streams.
     */
    private void circles() {
        canvas.getChildren().clear();
        Stream<Stream<Circle>> circles = Stream.generate(() -> this.makeRow()).limit(ROWS);
        row = 0;
        circles.forEach(r -> {
            addRowToCanvas(r);
        });
    }

    /**
     * This method creates a row of circles using Streams.
     * @return Stream <Circle> the row of circles
     */
    private Stream <Circle> makeRow() {
        return Stream.generate(() -> new Circle(RADIUS)).limit(COLS);
    }

    /**
     * This method adds adds the row of circles to the canvas.
     * @param s the stream of circles to add to the canvas
     */
    private void addRowToCanvas(Stream <Circle> s) {
        column = 0;
        s.forEach(c -> {
            addToCanvas(c);
        });
        row++;
    }

    /**
     * This method adds a circle to a position on the canvas.
     * @param c the circle to add
     */
    private void addToCanvas(Circle c) {
        c.setFill((Paint) new Color(Math.random(), Math.random(), Math.random(), 1));
        double toX = column * CELL_SIZE + (CELL_SIZE/2);
        double toY = row * CELL_SIZE + (CELL_SIZE/2);
        double fromX = (COLS*CELL_SIZE) - (CELL_SIZE/2);
        double fromY = (ROWS*CELL_SIZE) - (CELL_SIZE/2);
        c.setCenterX(fromX);
        c.setCenterY(fromY);
        canvas.getChildren().add((Circle)c);
        column++;
        TranslateTransition translate = new TranslateTransition(Duration.millis((double)500));
        translate.setNode((Node)c);
        translate.setByX(toX - fromX);
        translate.setByY(toY - fromY);
        translate.play();
        ScaleTransition scale = new ScaleTransition(Duration.millis((double)(500 * Math.random() + 500)));
        scale.setNode((Node)c);
        scale.setByX(2);
        scale.setByY(2);
        scale.setCycleCount(-1);
        scale.setAutoReverse(true);
        scale.play();
    }

    private VBox root;
    private Pane canvas;
    private Button starter;
    private int row;
    private int column;

    /**
     * @param args the command line arguments
     */
    public static void main(String... args) {
        launch(args);
    }
}
\$\endgroup\$
4
\$\begingroup\$

This looks very nice, straightforward code. A few minor improvements are possible.

Simplifications of lambdas

This uses a statement lambda:

starter.setOnAction(e -> {
    circles();
});

... can be replaced with an expression lambda:

starter.setOnAction(e -> circles());

This uses an expression lambda:

Stream<Stream<Circle>> circles = Stream.generate(() -> this.makeRow()).limit(ROWS);

... can be replaced with method reference:

Stream<Stream<Circle>> circles = Stream.generate(this::makeRow).limit(ROWS);

Similarly:

circles.forEach(r -> {
    addRowToCanvas(r);
});

Replace statement lambda with method reference:

circles.forEach(this::addRowToCanvas);

There are a few other places too where you can apply similar transformations.

Redundant casts

In addToCanvas you have several redundant casts, that can be simply dropped:

    c.setFill((Paint) new Color(Math.random(), Math.random(), Math.random(), 1));
    // ...
    canvas.getChildren().add((Circle) c);
    // ...

... and others.

Random numbers

Instead of Math.random(), it's better to use an instance of Random and its nextDouble() method. It's more modern, it can be seeded (for easier testing, for example), and passed as parameter, if ever needed.

Magic numbers

The number 500 appears at multiple places. If they are related, it would be good to put this into a constant.

Code organization

These variables are declared near the bottom of the cast:

private VBox root;
private Pane canvas;
private Button starter;
private int row;
private int column;

The recommended practices is to declare all field variables at the top of the class.

\$\endgroup\$
  • \$\begingroup\$ When I do Stream.generate(() -> new Circle(RADIUS)), can I somehow make that into a method reference since I'm calling the constructor method? \$\endgroup\$ – syb0rg Apr 20 '16 at 3:45
  • \$\begingroup\$ @janos Any thoughts about comparing Stream.generate(this::makeRow).limit(ROWS) with IntStream.range(0, ROWS).mapToObj(i -> this.makeRow())? \$\endgroup\$ – h.j.k. Apr 20 '16 at 8:10
  • \$\begingroup\$ @syb0rg if you called a parameterless constructor, you could write Stream.generate(Circle::new). But you need the RADIUS parameter, so you need the lambda expression in this case. \$\endgroup\$ – janos Apr 20 '16 at 19:56
  • \$\begingroup\$ @h.j.k. Stream.generate(this::makeRow).limit(ROWS) looks shorter and simpler. Is there a reason you would prefer the alternative you mentioned? \$\endgroup\$ – janos Apr 20 '16 at 19:59
  • 1
    \$\begingroup\$ @h.j.k. Excellent point to read the API docs! Having read it now, .limit can be expensive on ordered parallel pipelines, but here we have an unordered stream source (generate(Supplier)), so it should be fine. A second part of your question seems to ask if it's better to create a finite stream in the first place, rather than infinite + convert to finite later. If the API docs doesn't express a concern (I cannot see), then we don't need to speculate. Correct me if I'm wrong, I'm no expert of streams! \$\endgroup\$ – janos Apr 21 '16 at 4:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.