2
\$\begingroup\$

I'm learning JavaFX and how to make good code in general. I created a little aplication that let you calculate how many points cost to level up a character stats. But I noticed that I usually repeteat a good amount of code. Let's see what you guys think.

Here's the Main.class

import javafx.application.Application;
import javafx.fxml.FXMLLoader;
import javafx.scene.Parent;
import javafx.scene.Scene;
import javafx.stage.Stage;

public class Main extends Application {

    @Override
    public void start(Stage primaryStage) throws Exception{
        Parent root = FXMLLoader.load(getClass().getResource("sample.fxml"));
        primaryStage.setTitle("Calculadora de grajeas");
        primaryStage.setScene(new Scene(root, 600, 400));
        primaryStage.show();
    }


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

Here's the fxml:

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

<?import com.jfoenix.controls.JFXButton?>
<?import com.jfoenix.controls.JFXTextArea?>
<?import com.jfoenix.controls.JFXTextField?>
<?import javafx.geometry.Insets?>
<?import javafx.scene.control.Label?>
<?import javafx.scene.layout.AnchorPane?>
<?import javafx.scene.layout.ColumnConstraints?>
<?import javafx.scene.layout.GridPane?>
<?import javafx.scene.layout.RowConstraints?>

<AnchorPane maxHeight="-Infinity" maxWidth="-Infinity" minHeight="-Infinity" minWidth="-Infinity" prefHeight="400.0" prefWidth="600.0" style="-fx-background-color: #364A5A;" xmlns="http://javafx.com/javafx/9.0.1" xmlns:fx="http://javafx.com/fxml/1" fx:controller="sample.Controller">
   <children>
      <AnchorPane prefHeight="400.0" prefWidth="300.0" style="-fx-background-color: #4E6B81;" AnchorPane.bottomAnchor="0.0" AnchorPane.leftAnchor="0.0" AnchorPane.topAnchor="0.0">
         <children>
            <GridPane prefHeight="295.0" prefWidth="290.0" AnchorPane.leftAnchor="5.0" AnchorPane.rightAnchor="5.0" AnchorPane.topAnchor="35.0">
              <columnConstraints>
                  <ColumnConstraints hgrow="NEVER" minWidth="10.0" prefWidth="100.0" />
                <ColumnConstraints hgrow="SOMETIMES" minWidth="10.0" prefWidth="10.0" />
                <ColumnConstraints hgrow="SOMETIMES" maxWidth="68.0" minWidth="10.0" prefWidth="25.0" />
                  <ColumnConstraints hgrow="SOMETIMES" minWidth="10.0" prefWidth="10.0" />
              </columnConstraints>
              <rowConstraints>
                <RowConstraints minHeight="10.0" prefHeight="20.0" vgrow="SOMETIMES" />
                <RowConstraints minHeight="10.0" prefHeight="20.0" vgrow="SOMETIMES" />
                <RowConstraints minHeight="10.0" prefHeight="20.0" vgrow="SOMETIMES" />
                  <RowConstraints minHeight="10.0" prefHeight="20.0" vgrow="SOMETIMES" />
                  <RowConstraints minHeight="10.0" prefHeight="20.0" vgrow="SOMETIMES" />
                  <RowConstraints minHeight="10.0" prefHeight="20.0" vgrow="SOMETIMES" />
                  <RowConstraints minHeight="10.0" prefHeight="20.0" vgrow="SOMETIMES" />
                  <RowConstraints minHeight="10.0" prefHeight="20.0" vgrow="SOMETIMES" />
              </rowConstraints>
               <children>
                  <Label text="Impacto físico" textFill="WHITE" />
                  <Label text="Defensa física" textFill="WHITE" GridPane.rowIndex="1" />
                  <Label text="Impacto mágico" textFill="WHITE" GridPane.rowIndex="2" />
                  <JFXTextField fx:id="if_b" focusColor="WHITE" unFocusColor="#2abbe8" GridPane.columnIndex="1" />
                  <JFXTextField fx:id="df_b" focusColor="WHITE" unFocusColor="#2abbe8" GridPane.columnIndex="1" GridPane.rowIndex="1" />
                  <JFXTextField fx:id="im_b" focusColor="WHITE" unFocusColor="#2abbe8" GridPane.columnIndex="1" GridPane.rowIndex="2" />
                  <JFXTextField fx:id="if_a" focusColor="WHITE" prefHeight="25.0" prefWidth="80.0" unFocusColor="#2abbe8" GridPane.columnIndex="3" />
                  <JFXTextField fx:id="df_a" focusColor="WHITE" unFocusColor="#2abbe8" GridPane.columnIndex="3" GridPane.rowIndex="1" />
                  <JFXTextField fx:id="im_a" focusColor="WHITE" unFocusColor="#2abbe8" GridPane.columnIndex="3" GridPane.rowIndex="2" />
                  <JFXTextField fx:id="dm_b" focusColor="WHITE" unFocusColor="#2abbe8" GridPane.columnIndex="1" GridPane.rowIndex="3" />
                  <JFXTextField fx:id="dm_a" focusColor="WHITE" unFocusColor="#2abbe8" GridPane.columnIndex="3" GridPane.rowIndex="3" />
                  <JFXTextField fx:id="in_b" focusColor="WHITE" unFocusColor="#2abbe8" GridPane.columnIndex="1" GridPane.rowIndex="4" />
                  <JFXTextField fx:id="r_b" focusColor="WHITE" unFocusColor="#2abbe8" GridPane.columnIndex="1" GridPane.rowIndex="5" />
                  <JFXTextField fx:id="hm_b" focusColor="WHITE" unFocusColor="#2abbe8" GridPane.columnIndex="1" GridPane.rowIndex="6" />
                  <JFXTextField fx:id="v_b" focusColor="WHITE" unFocusColor="#2abbe8" GridPane.columnIndex="1" GridPane.rowIndex="7" />
                  <JFXTextField fx:id="in_a" focusColor="WHITE" unFocusColor="#2abbe8" GridPane.columnIndex="3" GridPane.rowIndex="4" />
                  <JFXTextField fx:id="r_a" focusColor="WHITE" unFocusColor="#2abbe8" GridPane.columnIndex="3" GridPane.rowIndex="5" />
                  <JFXTextField fx:id="hm_a" focusColor="WHITE" unFocusColor="#2abbe8" GridPane.columnIndex="3" GridPane.rowIndex="6" />
                  <JFXTextField fx:id="v_a" focusColor="WHITE" unFocusColor="#2abbe8" GridPane.columnIndex="3" GridPane.rowIndex="7" />
                  <Label text="Defensa mágica" textFill="WHITE" GridPane.rowIndex="3" />
                  <Label text="Iniciativa" textFill="WHITE" GridPane.rowIndex="4" />
                  <Label text="Resistencia" textFill="WHITE" GridPane.rowIndex="5" />
                  <Label text="Habilidad mágica" textFill="WHITE" GridPane.rowIndex="6" />
                  <Label text="Vitalidad" textFill="WHITE" GridPane.rowIndex="7" />
               </children>
               <padding>
                  <Insets left="15.0" right="15.0" />
               </padding>
            </GridPane>
            <JFXButton layoutX="234.0" layoutY="351.0" onAction="#calcularGrajeas" text="Calcular" textFill="WHITE" />
         </children>
      </AnchorPane>
      <AnchorPane layoutX="400.0" prefHeight="400.0" prefWidth="300.0" AnchorPane.bottomAnchor="0.0" AnchorPane.rightAnchor="0.0" AnchorPane.topAnchor="0.0">
         <children>
            <JFXTextArea fx:id="outputText" prefHeight="400.0" prefWidth="300.0" />
         </children>
      </AnchorPane>
   </children>
</AnchorPane>

And here's the Controller.class

import com.jfoenix.controls.JFXTextArea;
import com.jfoenix.controls.JFXTextField;
import javafx.fxml.FXML;


public class Controller {

    @FXML
    private JFXTextArea outputText;

    @FXML
    private JFXTextField if_b;

    @FXML
    private JFXTextField if_a;

    @FXML
    private JFXTextField df_b;

    @FXML
    private JFXTextField df_a;

    @FXML
    private JFXTextField im_b;

    @FXML
    private JFXTextField im_a;

    @FXML
    private JFXTextField dm_b;

    @FXML
    private JFXTextField dm_a;

    @FXML
    private JFXTextField in_b;

    @FXML
    private JFXTextField in_a;

    @FXML
    private JFXTextField r_b;

    @FXML
    private JFXTextField r_a;

    @FXML
    private JFXTextField hm_b;

    @FXML
    private JFXTextField hm_a;

    @FXML
    private JFXTextField v_b;

    @FXML
    private JFXTextField v_a;


    @FXML
    private void initialize() {

    }

    @FXML
    private void calcularGrajeas() {
        StringBuilder stringBuilder = new StringBuilder();
        int total_points = 0;


        if (!(if_b.getText().isEmpty() || if_a.getText().isEmpty())) {
            int before = Integer.valueOf(if_b.getText());
            int after = Integer.valueOf(if_a.getText());
            int points = 0;


            for (int i = before; i < after; i++) {
                if( i == 0){
                    points = points + 30;
                }else {
                    points = points+ 30 * i;
                }
            }

            total_points += points;
            stringBuilder.append(String.format("Strenght: %d\n", points));
        }

        if (!(df_b.getText().isEmpty() || df_a.getText().isEmpty())){

            int before = Integer.valueOf(dm_b.getText());
            int after = Integer.valueOf(df_a.getText());
            int points = 0;


            for (int i = before; i < after; i++) {
                if( i == 0){
                    points = points + 30;
                }else {
                    points = points+ 30 * i;
                }
            }

            total_points += points;
            stringBuilder.append(String.format("Defense: %d\n",points));
        }

        if (!(im_b.getText().isEmpty() || im_a.getText().isEmpty())){

            int before = Integer.valueOf(im_b.getText());
            int after = Integer.valueOf(im_a.getText());
            int points = 0;


            for (int i = before; i < after; i++) {
                if( i == 0){
                    points = points + 50;
                }else {
                    points = points+ 50 * i;
                }
            }

            total_points += points;
            stringBuilder.append(String.format("Magic: %d\n",points));
        }

        if (!(dm_b.getText().isEmpty() || dm_a.getText().isEmpty())){

            int before = Integer.valueOf(dm_b.getText());
            int after = Integer.valueOf(dm_a.getText());
            int points = 0;


            for (int i = before; i < after; i++) {
                if( i == 0){
                    points = points + 50;
                }else {
                    points = points+ 50 * i;
                }
            }

            total_points += points;
            stringBuilder.append(String.format("Magic Defense: %d\n",points));
        }

        if (!(in_b.getText().isEmpty() || in_a.getText().isEmpty())){

            int before = Integer.valueOf(in_b.getText());
            int after = Integer.valueOf(in_a.getText());
            int points = 0;


            for (int i = before; i < after; i++) {
                if( i == 0){
                    points = points + 50;
                }else {
                    points = points+ 50 * i;
                }
            }

            total_points += points;
            stringBuilder.append(String.format("Iniciative: %d\n",points));
        }

        if (!(r_b.getText().isEmpty() || r_a.getText().isEmpty())){

            int before = Integer.valueOf(r_b.getText());
            int after = Integer.valueOf(r_a.getText());
            int points = 0;


            for (int i = before; i < after; i++) {
                if( i == 0){
                    points = points + 40;
                }else {
                    points = points+ 40 * i;
                }
            }

            total_points += points;
            stringBuilder.append(String.format("Resistence: %d\n",points));
        }

        if (!(hm_b.getText().isEmpty() || hm_a.getText().isEmpty())){

            int before = Integer.valueOf(hm_b.getText());
            int after = Integer.valueOf(hm_a.getText());
            int points = 0;


            for (int i = before; i < after; i++) {
                if( i == 0){
                    points = points + 50;
                }else {
                    points = points+ 50 * i;
                }
            }

            total_points += points;
            stringBuilder.append(String.format("Magic ability: %d\n",points));
        }

        if (!(v_b.getText().isEmpty() || v_a.getText().isEmpty())){

            int before = Integer.valueOf(v_b.getText());
            int after = Integer.valueOf(v_a.getText());
            int points = 0;


            for (int i = before; i < after; i++) {
                if( i == 0){
                    points = points + 10;
                }else {
                    points = points+ 10 * i;
                }
            }

            total_points += points;
            stringBuilder.append(String.format("Vitality: %d\n",points));
        }

        stringBuilder.append(String.format("Total points: %d", total_points));
        outputText.setText(stringBuilder.toString());

    }
}

Any tips about the code, formatting, what type of comments would be good to put, etc would be apreciated.

\$\endgroup\$
1
\$\begingroup\$

Thanks for sharing your code.

Explicitly instantiate the controller

You use implicit instantiation of the controller class by adding the attribute fx:controller="sample.Controller" to your FXML.

This has the disadvantage that the controller class must have a default constructor and any dependencies (like provided business logic (e.g. database access) or the data model (in reference to the M from the MVVC pattern that JavaFX incorporates)) have to be accesses in a static way either by invoking the new operator or via the Singelton Pattern. That makes your code hard to reuse and therefore hard to be unittested.

You should instead instantiate the controller instance explicitly in the class Main:

@Override
public void start(Stage primaryStage) throws Exception{
    FXMLLoader fxmlLoader = new FXMLLoader();
    sample.Controller myController = new sample.Controller(/*pass dependencies here*/)
    // maybe some more configuration on myController 
    fxmlLoader.setController(myController);
    Parent root = flmlLoader.load(getClass().getResource("sample.fxml"));

    // alternatively the controller could know its FXML file:
    Parent root = flmlLoader.load(myController.getFxmlFileAsResource()));
    // ...

Enforce separation of concerns and same level of abstraction

Your controller class contains some calculation. But the (main) responsibility of a controller class is to organize the binding between the data model and the view model (the controllers properties annotated with @FXML) and triggering the business logic. Any business related calculation should be done bye some other class having that responsibility only.

The same level of abstraction principle requires that a class should either delegate calls to other classes or do (primitive) calculations. Therefore you should move the calculations done in calcularGrajeas() to a method in another class which you call in your method calcularGrajeas().

This suggestions might look like overkill for such a small project. But I believe one should start to get used to the right concepts as soon as possible...

\$\endgroup\$
  • \$\begingroup\$ Okay, I instantiated the controller in the Main class, and I will use the controller only to organize the view. Thanks for the tips! \$\endgroup\$ – Green_Sam Jan 14 '18 at 16:52
  • \$\begingroup\$ the more important point to me is: Did you understand why? \$\endgroup\$ – Timothy Truckle Jan 14 '18 at 17:07
3
\$\begingroup\$

I'm not familiar with JavaFX so I'll focus on things not related to it.

One thing that I would not recommend is to mix two languages when naming function (or anything really) in your language. You name every variable in English and then you use calcularGrajeas which is Spanish I guess? That's not really a good habit (I used to do the same, and it lead to confusion, and is harder to read for other people).

Still about naming, you have a lot of variables if_b, if_a, df_b .etc. It's really unclear what they stand for. My guess is _b and _a stand for before and after? You should use longer names, to help with clarity. (edit: as suggested by @TimothyTruckles, the link to the Java naming convention is here)

And then calcularGrajeas is very long, repeating a lot of code. One way to refactor it is as follow:

Define a function to calculate the points for one attribute (here I use JFXTextField as arguments, but you could use int with little modifications )

int calculatePointsAttribute(JFXTextField attribute_before, JFXTextField attribute_after)
{
    int points = 0;

    int before = Integer.valueOf(attribute_before.getText());
    int after = Integer.valueOf(attribute_after.getText());


    for (int i = before; i < after; i++) {
        if( i == 0){
            points = points + 30;
        }else {
            points = points+ 30 * i;
        }
    }

    return points;
}

And then use it in your calcularGrajeas function:

private void calcularGrajeas() {
        StringBuilder stringBuilder = new StringBuilder();
        int total_points = 0;
        int points = 0;


        if (!(if_b.getText().isEmpty() || if_a.getText().isEmpty())) {
            points = calculatePointsAttribute(if_b, if_a);
            total_points += points;
            stringBuilder.append(String.format("Strenght: %d\n", points));
        }

        if (!(df_b.getText().isEmpty() || df_a.getText().isEmpty())){
            points = calculatePointsAttribute(df_b, df_a);
            total_points += points;
            stringBuilder.append(String.format("Defense: %d\n",points));
        }

        if (!(im_b.getText().isEmpty() || im_a.getText().isEmpty())){
            points = calculatePointsAttribute(im_b, im_a);
            total_points += points;
            stringBuilder.append(String.format("Magic: %d\n",points));
        }

        if (!(dm_b.getText().isEmpty() || dm_a.getText().isEmpty())){
            points = calculatePointsAttribute(dm_b, dm_a);
            total_points += points;
            stringBuilder.append(String.format("Magic Defense: %d\n",points));
        }

        if (!(in_b.getText().isEmpty() || in_a.getText().isEmpty())){
            points = calculatePointsAttribute(in_b, in_a);
            total_points += points;
            stringBuilder.append(String.format("Iniciative: %d\n",points));
        }

        if (!(r_b.getText().isEmpty() || r_a.getText().isEmpty())){
            points = calculatePointsAttribute(r_b, r_a);
            total_points += points;
            stringBuilder.append(String.format("Resistence: %d\n",points));
        }

        if (!(hm_b.getText().isEmpty() || hm_a.getText().isEmpty())){
            points = calculatePointsAttribute(hm_b, hm_a);
            total_points += points;
            stringBuilder.append(String.format("Magic ability: %d\n",points));
        }

        if (!(v_b.getText().isEmpty() || v_a.getText().isEmpty())){
            points = calculatePointsAttribute(v_b, v_a);
            total_points += points;
            stringBuilder.append(String.format("Vitality: %d\n",points));
        }

        stringBuilder.append(String.format("Total points: %d", total_points));
        outputText.setText(stringBuilder.toString());

}

EDIT:

One other change you could consider if changing the logic a little bit isnt too much a problem is to define:

int calculatePointsAttribute(JFXTextField attribute_before, JFXTextField attribute_after)
{
    int points = 0;

    if (!(v_b.getText().isEmpty() || v_a.getText().isEmpty())){
        int before = Integer.valueOf(attribute_before.getText());
        int after = Integer.valueOf(attribute_after.getText());


        for (int i = before; i < after; i++) {
            if( i == 0){
                points = points + 30;
            }else {
                points = points+ 30 * i;
            }
        }
    }

        return points;
    }

Then calcularGrajeas become:

private void calcularGrajeas() {
        StringBuilder stringBuilder = new StringBuilder();
        int total_points = 0;
        int points = 0;

        points = calculatePointsAttribute(if_b, if_a);
        total_points += points;
        stringBuilder.append(String.format("Strenght: %d\n", points));


        points = calculatePointsAttribute(df_b, df_a);
        total_points += points;
        stringBuilder.append(String.format("Defense: %d\n",points));


        points = calculatePointsAttribute(im_b, im_a);
        total_points += points;
        stringBuilder.append(String.format("Magic: %d\n",points));


        points = calculatePointsAttribute(dm_b, dm_a);
        total_points += points;
        stringBuilder.append(String.format("Magic Defense: %d\n",points));


        points = calculatePointsAttribute(in_b, in_a);
        total_points += points;
        stringBuilder.append(String.format("Iniciative: %d\n",points));


        points = calculatePointsAttribute(r_b, r_a);
        total_points += points;
        stringBuilder.append(String.format("Resistence: %d\n",points));

        points = calculatePointsAttribute(hm_b, hm_a);
        total_points += points;
        stringBuilder.append(String.format("Magic ability: %d\n",points));


        points = calculatePointsAttribute(v_b, v_a);
        total_points += points;
        stringBuilder.append(String.format("Vitality: %d\n",points));


        stringBuilder.append(String.format("Total points: %d", total_points));
        outputText.setText(stringBuilder.toString());

}

And then you can also put all :

points = calculatePointsAttribute(X, X);
total_points += points;
stringBuilder.append(String.format("X: %d\n",points));

into a loop and your good to go!

Nice project!

\$\endgroup\$
  • \$\begingroup\$ I really have to stop mixing languages, thanks for the tip! \$\endgroup\$ – Green_Sam Jan 14 '18 at 16:51
  • \$\begingroup\$ No problem, It's an habit that is hard to lose, but it pay off eventually. \$\endgroup\$ – Julien Rousé Jan 14 '18 at 19:29
  • \$\begingroup\$ @JulienRousé would you mind to add the Link to the Java Naming Conventions oracle.com/technetwork/java/codeconventions-135099.html ? \$\endgroup\$ – Timothy Truckle Jan 15 '18 at 11:24
  • \$\begingroup\$ @TimothyTruckle good idea, I'll add it \$\endgroup\$ – Julien Rousé Jan 15 '18 at 11:56

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.