HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

What makes a hero?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
makesherowhat

Problem

This is a simple program meant to show how dynamic JavaFX is. It moves one list item to another and back without having to update the screen directly, thanks to the Observable Collection classes. It also exemplifies how links would work.

In the same vein as Hello parallel world, I practiced and illustrated some ideas with the intention of teaching a friend later on. I'm curious about what could be improved, and if anything is done poorly or inefficiently.

```
import java.awt.Desktop;
import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;

import javafx.application.Application;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import javafx.geometry.HPos;
import javafx.geometry.Insets;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.scene.control.ListView;
import javafx.scene.layout.BorderPane;
import javafx.scene.layout.ColumnConstraints;
import javafx.scene.layout.GridPane;
import javafx.scene.layout.Priority;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;

public class Heroes extends Application {
public static void main(String[] args) {
launch(args);
}

@Override
public void start(Stage stage) {
ColumnConstraints column1 = new ColumnConstraints(150, 150, Double.MAX_VALUE);
column1.setHgrow(Priority.ALWAYS);

ColumnConstraints column2 = new ColumnConstraints(60);

ColumnConstraints column3 = new ColumnConstraints(150, 150, Double.MAX_VALUE);
column3.setHgrow(Priority.ALWAYS);

GridPane gridpane = new GridPane();
gridpane.setPadding(new Insets(5));
gridpane.setHgap(10);
gridpane.setVgap(10);
gridpane.getColumnConstraints().addAll(column1, column2, column3);

Label candidatesLabel = new Label("Candidates");
GridPane.setHalignment(candidatesLabel, HPos.CENTER);
gridpane.add(candidatesLabel, 0, 0);

Solution

2 male and 2 female superheros... Very politically correct ;-)

I suggest to try to avoid repeated code as much as possible.
For example,
Desktop.getDesktop().browse(...) is used in most of the case statements,
only the URL parameter is different.
I would refactor this in a way that the case statements set a URL variable,
so that you can write the Desktop.getDesktop().browse(...) call one time,
after the switch.

Of course, this won't apply to Code Reviewers.
In that case, return from the switch,
instead of setting a URL.

Another kind of duplication in your code is that of the names of superheros:
they appear first when you add them to the candidates list,
and then again in the case statements.
As such, it seems it would be better to use a Map (or Map,
and do without the switch.

Context

StackExchange Code Review Q#93572, answer score: 7

Revisions (0)

No revisions yet.