4
\$\begingroup\$

I am currently working on a project in Java, and I use JavaFX for the GUI of the System. Currently, I am in the designing phase of the system, where I am designing the look of the system as well as setting up the basic functionality like navigation. I would highly appreciate it if someone can review my simple navigation system and the code I have written so far. I would like to know:

  • Any bad practices I am following, and how would I improve them.
  • Any architectural issues in my program, and how would I rectify that issue.
  • Any techniques I could follow to keep my software more maintainable.
  • Any inefficiencies that can be optimised.
  • And any sort of points you have in your mind that I could follow to improve my software.
  • Anything that you can add to the list above would also highly help.

I also have a preloader for this application, I have provided the code just for the sake of completeness. This preloader was generated by NetBeans.

TeleMart_Preloader.java

/*
 * Program developed by Hassan Althaf.
 * Copyright © 2015, Hassan Althaf.
 * Website: http://hassanalthaf.com
 */
package telemart.preloader;

import javafx.application.Preloader;
import javafx.application.Preloader.ProgressNotification;
import javafx.application.Preloader.StateChangeNotification;
import javafx.scene.Scene;
import javafx.scene.control.ProgressBar;
import javafx.scene.layout.BorderPane;
import javafx.stage.Stage;

/**
 * Simple Preloader Using the ProgressBar Control
 *
 * @author hassan
 */
public class TeleMart_Preloader extends Preloader {

    ProgressBar bar;
    Stage stage;

    private Scene createPreloaderScene() {
        bar = new ProgressBar();
        BorderPane p = new BorderPane();
        p.setCenter(bar);
        return new Scene(p, 300, 150);        
    }

    @Override
    public void start(Stage stage) throws Exception {
        this.stage = stage;
        stage.setScene(createPreloaderScene());        
        stage.show();
    }

    @Override
    public void handleStateChangeNotification(StateChangeNotification scn) {
        if (scn.getType() == StateChangeNotification.Type.BEFORE_START) {
            stage.hide();
        }
    }

    @Override
    public void handleProgressNotification(ProgressNotification pn) {
        bar.setProgress(pn.getProgress());
    }    

}

com.hassanalthaf.telemart.Main.java:

/*
 * Program developed by Hassan Althaf.
 * Copyright © 2015, Hassan Althaf.
 * Website: http://hassanalthaf.com
 */
package com.hassanalthaf.telemart;

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

/**
 *
 * @author hassan
 */
public class Main extends Application {

    public static final String APPLICATION_TITLE = "TeleMart - ERP System";

    @Override
    public void start(Stage stage) throws Exception {
        Parent root = FXMLLoader.load(getClass().getResource("views/MainView.fxml"));

        Scene scene = new Scene(root);

        stage.setScene(scene);
        stage.setResizable(false);
        stage.setTitle(Main.APPLICATION_TITLE);
        stage.show();
    }

    /**
     * @param args the command line arguments
     */
    public static void main(String[] args) {
        launch(args);
    }

}

com.hassanalthaf.telemart.views.Dashboard.fxml:

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

<?import javafx.scene.shape.*?>
<?import java.lang.*?>
<?import java.util.*?>
<?import javafx.scene.*?>
<?import javafx.scene.control.*?>
<?import javafx.scene.layout.*?>

<AnchorPane id="AnchorPane" fx:id="dashboard" prefHeight="400.0" prefWidth="600.0" stylesheets="@css/dashboard.css" xmlns="http://javafx.com/javafx/8" xmlns:fx="http://javafx.com/fxml/1" fx:controller="com.hassanalthaf.telemart.viewmodels.DashboardViewModel">
   <children>
      <MenuBar maxWidth="600.0" minWidth="600.0" prefWidth="600.0">
        <menus>
          <Menu mnemonicParsing="false" text="File">
            <items>
              <MenuItem fx:id="homeMenuItem" mnemonicParsing="false" onAction="#menuItemClick" text="Home" />
                  <MenuItem fx:id="anotherPageMenuItem" mnemonicParsing="false" onAction="#menuItemClick" text="Another Page" />
                  <MenuItem fx:id="differentPageMenuItem" mnemonicParsing="false" onAction="#menuItemClick" text="Different Page" />
            </items>
          </Menu>
        </menus>
      </MenuBar>
      <AnchorPane fx:id="differentPage" layoutY="29.0" maxHeight="-Infinity" maxWidth="-Infinity" minHeight="-Infinity" minWidth="-Infinity" opacity="0.0" prefHeight="371.0" prefWidth="600.0">
         <children>
            <TextArea layoutX="155.0" layoutY="62.0" prefHeight="200.0" prefWidth="200.0" promptText="Page three" />
         </children>
      </AnchorPane>
      <AnchorPane fx:id="anotherPage" layoutY="29.0" maxHeight="-Infinity" maxWidth="-Infinity" minHeight="-Infinity" minWidth="-Infinity" opacity="0.0" prefHeight="371.0" prefWidth="600.0">
         <children>
            <TextField layoutX="180.0" layoutY="77.0" promptText="Page Two" />
         </children>
      </AnchorPane>
      <AnchorPane fx:id="home" layoutY="29.0" maxHeight="-Infinity" maxWidth="-Infinity" minHeight="-Infinity" minWidth="-Infinity" prefHeight="371.0" prefWidth="600.0">
         <children>
            <TextField layoutX="133.0" layoutY="150.0" text="First Page" />
         </children>
      </AnchorPane>
   </children>
</AnchorPane>

com.hassanalthaf.telemart.views.MainView.fxml:

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

<?import java.lang.*?>
<?import java.util.*?>
<?import javafx.scene.*?>
<?import javafx.scene.control.*?>
<?import javafx.scene.layout.*?>

<AnchorPane id="AnchorPane" fx:id="mainWindow" maxHeight="-Infinity" maxWidth="-Infinity" minHeight="-Infinity" minWidth="-Infinity" prefHeight="350.0" prefWidth="600.0" stylesheets="@css/style.css" xmlns="http://javafx.com/javafx/8" xmlns:fx="http://javafx.com/fxml/1" fx:controller="com.hassanalthaf.telemart.viewmodels.MainViewModel">
   <children>
      <Label layoutX="222.0" layoutY="35.0" styleClass="title" text="TeleMart" />
      <TextField layoutX="380.0" layoutY="160.0" onKeyPressed="#loginEnter" promptText="Username">
         <styleClass>
            <String fx:value="login-field" />
            <String fx:value="login-field" />
         </styleClass>
      </TextField>
      <PasswordField layoutX="380.0" layoutY="208.0" onKeyPressed="#loginEnter" promptText="Password" styleClass="login-field" />
      <Button layoutX="495.0" layoutY="256.0" mnemonicParsing="false" onMouseClicked="#loginClick" text="Login" />
      <Label layoutX="45.0" layoutY="113.0" styleClass="content" text="Welcome to TeleMart's Enterprise&#10;Resource Planning System.&#10;Please enter your login credentials&#10;on the right so that we could verify&#10;your identity. Also, please do not&#10;share your login credentials with&#10;anyone or use another person's&#10;login credentials." textAlignment="CENTER" wrapText="true" />
      <Label layoutX="220.0" layoutY="309.0" styleClass="content" text="© 2015, Hassan Althaf." />
   </children>
</AnchorPane>

com.hassanalthaf.telemart.views.css.style.css:

/*
Program developed by Hassan Althaf.
Copyright © 2015, Hassan Althaf.
Website: http://hassanalthaf.com
*/
/* 
    Created on : Dec 19, 2015, 6:09:52 PM
    Author     : hassan
*/

@font-face {
    font-family: 'Lato-Regular';
    src: url('../fonts/Lato-Regular.ttf');
}

@font-face {
    font-family: 'Lato-Hairline';
    src: url('../fonts/Lato-Hairline.ttf');
}

.root {
    -fx-background-color: #2C3E50;
}

.title {
    -fx-font-family: 'Lato-Hairline';
    -fx-font-size: 30pt;
    -fx-text-fill: #FFFFFF;
}

.login-field {
    -fx-background-color: #FFFFFF;
    -fx-border-radius: 5pt;
    -fx-padding: 10px;
    -fx-border-width: 1pt;
    -fx-border-style: solid;
    -fx-border-color: #202D3A;
    -fx-background-insets: 2, 0, 0;
    -fx-font-family: 'Lato-Regular';
}

.button {
    -fx-background-color: #3498DB;
    -fx-text-fill: #FFFFFF;
    -fx-font-size: 11pt;
    -fx-padding: 8px;
    -fx-border-radius: 10pt;
}

.button:hover {
    -fx-background-color: #51A7E0;
}

.content {
    -fx-font-family: 'Lato-Regular';
    -fx-font-size: 11pt;
    -fx-text-fill: #FFFFFF;
}

com.hassanalthaf.telemart.views.css.dashboard.css:

/*
Program developed by Hassan Althaf.
Copyright © 2015, Hassan Althaf.
Website: http://hassanalthaf.com
*/
/* 
    Created on : Dec 19, 2015, 11:29:50 PM
    Author     : hassan
*/
@font-face {
    font-family: 'Lato-Regular';
    src: url('../fonts/Lato-Regular.ttf');
}

.menu-bar {
    -fx-background-color: #3498DB;
}

.menu:hover {
    -fx-background-color: #51A7E0;
}

.menu:showing {
    -fx-background-color: #2487C9;
}

.menu .label {
    -fx-text-fill: #FFFFFF;
    -fx-font-family: 'Lato-Regular';
    -fx-font-size: 10pt;
}

.menu-item {
    -fx-background-color: #FFFFFF;
}

.menu-item:hover {
    -fx-background-color: #51A7E0;
}

.menu-item .label {
    -fx-text-fill: #333333;
    -fx-font-family: 'Lato-Regular';
    -fx-font-size: 10pt;
}

.menu-item:hover .label {
    -fx-text-fill: #FFFFFF;
}

com.hassanalthaf.telemart.viewmodels.MainViewModel.java:

/*
 * Program developed by Hassan Althaf.
 * Copyright © 2015, Hassan Althaf.
 * Website: http://hassanalthaf.com
 */
package com.hassanalthaf.telemart.viewmodels;

import java.io.IOException;
import java.net.URL;
import java.util.ResourceBundle;
import javafx.fxml.FXML;
import javafx.fxml.FXMLLoader;
import javafx.fxml.Initializable;
import javafx.scene.Parent;
import javafx.scene.input.KeyCode;
import javafx.scene.input.KeyEvent;
import javafx.scene.input.MouseEvent;

/**
 *
 * @author hassan
 */
public class MainViewModel implements Initializable {

    @FXML
    private Parent mainWindow;

    @FXML
    private void loginClick(MouseEvent event) {
        try {
            this.openDashboard();
        } catch (IOException exception) {

        }
    }

    @FXML
    private void loginEnter(KeyEvent event) {
        if (event.getCode() == KeyCode.ENTER) {
            try {
                this.openDashboard();
            } catch (IOException exception) {
                exception.printStackTrace();
            }
        }
    }

    private void openDashboard() throws IOException {
        FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("/com/hassanalthaf/telemart/views/Dashboard.fxml"));
        Parent dashboard = fxmlLoader.load();
        DashboardViewModel dashboardViewModel = fxmlLoader.getController();
        dashboardViewModel.show(this.mainWindow);
    }

    @Override
    public void initialize(URL url, ResourceBundle rb) {
        // TODO
    }

}

com.hassanalthaf.telemart.viewmodels.DashboardViewModel.java:

/*
 * Program developed by Hassan Althaf.
 * Copyright © 2015, Hassan Althaf.
 * Website: http://hassanalthaf.com
 */
package com.hassanalthaf.telemart.viewmodels;

import com.hassanalthaf.telemart.Main;
import java.net.URL;
import java.util.ResourceBundle;
import javafx.event.ActionEvent;
import javafx.fxml.FXML;
import javafx.fxml.Initializable;
import javafx.scene.Parent;
import javafx.scene.Scene;
import javafx.scene.control.MenuItem;
import javafx.scene.layout.AnchorPane;
import javafx.stage.Stage;

/**
 * FXML Controller class
 *
 * @author hassan
 */
public class DashboardViewModel implements Initializable {

    @FXML
    private Parent dashboard;

    @FXML
    private AnchorPane home;

    @FXML
    private AnchorPane anotherPage;

    @FXML
    private AnchorPane differentPage;

    private AnchorPane currentPage;

    private void changePage(AnchorPane page) {
        this.currentPage.setOpacity(0);
        this.currentPage = page;
        this.currentPage.toFront();
        this.currentPage.setOpacity(1);
    }

    public void menuItemClick(ActionEvent event) {
        Object source = (MenuItem)event.getSource();
        MenuItem clickedItem;

        if(source instanceof MenuItem) {
            clickedItem = (MenuItem)source;
        } else {
            return;
        }

        String id = clickedItem.getId();

        switch (id) {
            case "homeMenuItem":
                this.changePage(this.home);
                break;
            case "anotherPageMenuItem":
                this.changePage(this.anotherPage);
                break;
            case "differentPageMenuItem":
                this.changePage(this.differentPage);
                break;
            default:
                this.changePage(this.home);
                break;
        }
    }

    public void show(Parent main) {
        Scene scene = new Scene(this.dashboard);

        Stage stage = new Stage();
        stage.setScene(scene);
        stage.setTitle(Main.APPLICATION_TITLE);
        stage.setResizable(false);
        stage.show();

        Stage mainStage = (Stage)main.getScene().getWindow();
        mainStage.close();
    }

    /**
     * Initializes the controller class.
     */
    @Override
    public void initialize(URL url, ResourceBundle rb) {
        // TODO
        this.currentPage = this.home;
    }    

}

The entire source code with all the assets can be found here on GitHub.

\$\endgroup\$

2 Answers 2

1
+50
\$\begingroup\$

Since the preloader is autogenerated I won't nag about that one. I won't nag on Main either, since that's the default code to start any JavaFX application.

I can start nagging on your FXMLs though.

  • Any IDE worth it's salt will show you unnecessary inputs. Those should generally be removed.
    For Dashboard that's the first four, for MainView it's java.util.* and javafx.scene.*

  • While we're on imports: the JavaFX loader can benefit from not having to search a whole package. Use your IDE's "optimize imports" to avoid wildcard imports.

  • JavaFX has the wonderful idea of "default property tags". In general FXML gets a lot easier to read if you don't add the redundant default property tags and just continue as you know it.

  • It's also a generally good idea to minimize the amount of vertical scrolling when reading XML files. I have configured my IDE to 120 columns width, which is 50% more than the "terminal standard" of 80, and generally enough for 99% of code to not require vertical scrolling.

  • I wonder why you'd disable mnemoicParsing. It's generally accepted best practice to enable using the keyboard for Menu navigation. You should not force users into using the mouse in your App if you get keyboard navigation for free.

  • It's usually a not so great idea to force linebreaks in standard paragraph text as you do on the login screen. This just means more work for translators, additional cognitive load for people reading the code and it usually wastes tons of generally useful features from a layouting system. It also results in a generally bad user experience for users that have a larger system font (unless you fix the font size, which is also a bad experience).

While we're at these "UX" issues: Don't tell users to not share their credentials with anyone. It's really condescending.

And since I talked about users with larger system fonts: It's really bad style to define font sizes in pt or px. It's a horrendous system for both High-DPI users as well as people with difficulty reading. Usually the latter ones customize their system's fonts to be displayed larger. That will generally wreak havoc with any layouts that rely on text not taking more than a certain amount of space. Or it will not be properly applied for your app, which makes for a horrendous exerience. Instead of fixing the font size in px/pt, you should (just like in web design) specify font sizes in em.

Which brings me to my next point: Don't constrain the size of components in the FXML. The only thing you may constrain there is the ratios components have against one another. Read up on dynamically scaling designs. In general avoid absolute sizes for any and all components' width and height.

Furthermore use em (or rem) for sizing fonts and dynamic distances that should scale with the user's system font size. read up on it


Let's get on with the Java code for the ViewModels....

As Stan already outlined in his answer, you're not handling the IOException in loginClick. That's bad. The actual problem is a different one: you're letting that exception bubble, which is the actual cause of that inconsistency.

@FXML
private void loginClick(MouseEvent e) {
    openDashboard();
}

@FXML
private void loginEnter(KeyEvent e) {
    if (event.getCode() == KeyCode.ENTER) {
        openDashboard();
    }
}

private void openDashboard() {
    try {
        // see the difference?
    }
    // ...
}

While we're in that class: Don't implement from interfaces you don't use. Drop the implements Initializable and the public void initialize if you're not going to use it...

Final Words:

While it's often recommended, switching on the event target of an event is a terrible idea because it hides errors from you during development and leads to generally hard to debug issues. In addition to that I personally prefer to only define the layout of a view in the FXML, and the behaviour in the Controller.

This generally entails using the initialize method to bind action handlers to MenuItems and similar changes. It also stops your IDE from complaining about the fx:ids not resolving to backing properties on your Controller...

This is how the changed files look for me now:

Dashboard.fxml:

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

<?import javafx.scene.control.*?>
<?import javafx.scene.layout.AnchorPane?>

<AnchorPane id="AnchorPane" fx:id="dashboard" prefHeight="400.0" prefWidth="600.0" stylesheets="@css/dashboard.css"
            xmlns="http://javafx.com/javafx/8" xmlns:fx="http://javafx.com/fxml/1"
            fx:controller="com.hassanalthaf.telemart.viewmodels.DashboardViewModel">
    <MenuBar maxWidth="600.0" minWidth="600.0" prefWidth="600.0">
        <Menu mnemonicParsing="false" text="File">
            <MenuItem fx:id="homeMenuItem" text="Home"/>
            <MenuItem fx:id="anotherPageMenuItem" text="Another Page"/>
            <MenuItem fx:id="differentPageMenuItem" text="Different Page"/>
        </Menu>
    </MenuBar>
    <AnchorPane fx:id="differentPage" layoutY="29.0" maxHeight="-Infinity" maxWidth="-Infinity" minHeight="-Infinity"
                minWidth="-Infinity" opacity="0.0" prefHeight="371.0" prefWidth="600.0">
        <TextArea layoutX="155.0" layoutY="62.0" prefHeight="200.0" prefWidth="200.0" promptText="Page three"/>
    </AnchorPane>
    <AnchorPane fx:id="anotherPage" layoutY="29.0" maxHeight="-Infinity" maxWidth="-Infinity" minHeight="-Infinity"
                minWidth="-Infinity" opacity="0.0" prefHeight="371.0" prefWidth="600.0">
        <TextField layoutX="180.0" layoutY="77.0" promptText="Page Two"/>
    </AnchorPane>
    <AnchorPane fx:id="home" layoutY="29.0" maxHeight="-Infinity" maxWidth="-Infinity" minHeight="-Infinity"
                minWidth="-Infinity" prefHeight="371.0" prefWidth="600.0">
        <TextField layoutX="133.0" layoutY="150.0" text="First Page"/>
    </AnchorPane>
</AnchorPane>

DashboardViewModel.java

/*
 * Program developed by Hassan Althaf.
 * Copyright © 2015, Hassan Althaf.
 * Website: http://hassanalthaf.com
 */
package com.hassanalthaf.telemart.viewmodels;

import com.hassanalthaf.telemart.Main;

import java.net.URL;
import java.util.ResourceBundle;

import javafx.fxml.FXML;
import javafx.fxml.Initializable;
import javafx.scene.Parent;
import javafx.scene.Scene;
import javafx.scene.control.MenuItem;
import javafx.scene.layout.AnchorPane;
import javafx.stage.Stage;

/**
 * FXML Controller class
 *
 * @author hassan
 */
public class DashboardViewModel implements Initializable {

    @FXML
    public MenuItem homeMenuItem;
    @FXML
    public MenuItem anotherPageMenuItem;
    @FXML
    public MenuItem differentPageMenuItem;

    @FXML
    private Parent dashboard;

    @FXML
    private AnchorPane home;
    @FXML
    private AnchorPane anotherPage;
    @FXML
    private AnchorPane differentPage;

    private AnchorPane currentPage;

    private void changePage(AnchorPane page) {
        this.currentPage.setOpacity(0);
        this.currentPage = page;
        this.currentPage.toFront();
        this.currentPage.setOpacity(1);
    }

    public void show(Parent main) {
        Scene scene = new Scene(this.dashboard);

        Stage stage = new Stage();
        stage.setScene(scene);
        stage.setTitle(Main.APPLICATION_TITLE);
        stage.setResizable(false);
        stage.show();

        Stage mainStage = (Stage) main.getScene().getWindow();
        mainStage.close();
    }

    /**
     * Initializes the controller class.
     */
    @Override
    public void initialize(URL url, ResourceBundle rb) {
        currentPage = home;

        homeMenuItem.setOnAction((e) -> changePage(home));
        anotherPageMenuItem.setOnAction((e) -> changePage(anotherPage));
        differentPageMenuItem.setOnAction((e) -> changePage(differentPage));
    }
}

MainViewModel.java

/*
 * Program developed by Hassan Althaf.
 * Copyright © 2015, Hassan Althaf.
 * Website: http://hassanalthaf.com
 */
package com.hassanalthaf.telemart.viewmodels;

import javafx.fxml.FXML;
import javafx.fxml.FXMLLoader;
import javafx.scene.Parent;
import javafx.scene.input.KeyCode;
import javafx.scene.input.KeyEvent;
import javafx.scene.input.MouseEvent;

import java.io.IOException;

/**
 *
 * @author hassan
 */
public class MainViewModel {

    @FXML
    private Parent mainWindow;

    @FXML
    private void loginClick(MouseEvent event) {
            this.openDashboard();
    }

    @FXML
    private void loginEnter(KeyEvent event) {
        if (event.getCode() == KeyCode.ENTER) {
                this.openDashboard();
        }
    }

    private void openDashboard() {
        FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("/com/hassanalthaf/telemart/views/Dashboard.fxml"));
        try {
            Parent dashboard = fxmlLoader.load();
        } catch (IOException e) {
            e.printStackTrace();
            return;
        }
        DashboardViewModel dashboardViewModel = fxmlLoader.getController();
        dashboardViewModel.show(this.mainWindow);
    }
}

I have not changed anything about the css, Main or the Preloader. Finally I have one small nitpick... In the login view the button and the fields don't line up. That's terribly irritating ;)

\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot!!!! I don't really remember why I disabled mnemonic parsing since it's been a while. :( \$\endgroup\$
    – codez
    Commented Oct 5, 2017 at 9:09
3
\$\begingroup\$

No need to include class identifier

stage.setTitle(APPLICATION_TITLE);

Unhandled exception:

  @FXML
        private void loginClick(MouseEvent event) {
            try {
                this.openDashboard();
            } catch (IOException exception) {

            }
        }

For your menu item handling, I really suggest using an enum instead of strings as identifiers.

    String id = clickedItem.getId();

    switch (id) {
        case "homeMenuItem":
            this.changePage(this.home);
            break;
        case "anotherPageMenuItem":
            this.changePage(this.anotherPage);
            break;
        case "differentPageMenuItem":
            this.changePage(this.differentPage);
            break;
        default:
            this.changePage(this.home);
            break;
    }

https://docs.oracle.com/javase/tutorial/java/javaOO/enum.html

Also, you use this for non-static object referencing in situations where it's not necessary. You should only use this when you want to reference a non-static object's field that has the same name as a local variable. https://stackoverflow.com/questions/3728062/what-is-the-meaning-of-this-in-java

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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