5
\$\begingroup\$

I have programmed this simple Stock Control System using JavaFX for GUI and PostgreSQL for a database.

Please go through my code and point out flaws, inefficiencies, and better ways of doing things. I'd also like to be made aware of best practices which I am not following. The last two files are external links because I exceed the character limit (but don't review them).

com.HassanAlthaf.StockControlSystem.Main.java

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

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 stage) throws Exception {
        Parent root = FXMLLoader.load(getClass().getResource("View/LoginWindow.fxml"));

        Scene scene = new Scene(root);

        stage.setScene(scene);

        stage.setTitle("Authentication Required");
        stage.setResizable(false);

        stage.show();
    }

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

com.HassanAlthaf.StockControlSystem.Database.DatabaseAdapter.java

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

import java.io.File;
import java.io.FileReader;
import java.sql.Connection;
import java.sql.DriverManager;
import java.util.Properties;

public class DatabaseAdapter {
    private Connection connection;
    private String port;
    private String databaseName;
    private String username;
    private String password;
    private String host;

    public DatabaseAdapter() {
        this.fetchAndSetConnectionDetails();
    }

    public Connection getConnection() {
        try {

            Class.forName("org.postgresql.Driver");
            this.connection = DriverManager.getConnection("jdbc:postgresql://" + this.host + ":" + this.port + "/" + this.databaseName, this.username, this.password);

        } catch(Exception exception) {
            exception.printStackTrace();
            System.exit(0);
        }

        return this.connection;
    }

    public void fetchAndSetConnectionDetails() {
        File databaseSettings = new File("databaseSettings.properties");

        try {
            FileReader reader = new FileReader(databaseSettings);
            Properties properties = new Properties();
            properties.load(reader);

            this.host = properties.getProperty("host");
            this.username = properties.getProperty("username");
            this.password = properties.getProperty("password");
            this.port = properties.getProperty("port");
            this.databaseName = properties.getProperty("databaseName");
        } catch(Exception exception) {
            exception.printStackTrace();
        }
    }
}

com.HassanAlthaf.StockControlSystem.Stocks.StockController.java

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

import java.sql.SQLException;
import java.util.ArrayList;

public class StockController {
    private final StockRepository stockRepository;
    private ArrayList<String> returnMessages;

    public StockController() {
        this.stockRepository = new StockRepository();
    }

    public boolean addStockItem(String productName, String availableQuantity, String unitPrice, String reorderLevel) throws SQLException {
        ArrayList<String> returnMessages = this.validateStockItemData(productName, availableQuantity, unitPrice, reorderLevel);

        int numericalAvailableQuantity = 0;
        int numericalReorderLevel = 0;
        double numericalUnitPrice = 0;

        try {
            numericalAvailableQuantity = Integer.parseInt(availableQuantity);
            numericalReorderLevel = Integer.parseInt(reorderLevel);
        } catch (NumberFormatException exception) {
            returnMessages.add("Available quantity and re-order level may only be whole numbers!");
        }

        try {
            numericalUnitPrice = Double.parseDouble(unitPrice);
        } catch (NumberFormatException exception) {
            returnMessages.add("The unit price may only contain numbers and a decimal point!");
        }

        if(returnMessages.isEmpty()) {
            StockItem stockItem = new StockItem(productName, numericalAvailableQuantity, numericalUnitPrice, numericalReorderLevel);
            this.stockRepository.addStockItem(stockItem);

            return true;
        }

        this.returnMessages = returnMessages;

        return false;
    }

    public ArrayList<String> validateStockItemData(String productName, String availableQuantity, String unitPrice, String reorderLevel) {
        ArrayList<String> returnMessages = new ArrayList<>();

        if(productName.length() < 3) {
            returnMessages.add("The name of your product must be at least 3 characters long!");
        }

        if(availableQuantity.length() == 0) {
            returnMessages.add("Please enter the available quantity!");
        }

        if(unitPrice.length() == 0) {
            returnMessages.add("Please enter a unit price!");
        }

        if(reorderLevel.length() == 0) {
            returnMessages.add("Please enter a re-order level!");
        }

        return returnMessages;
    }

    public ArrayList<String> getReturnMessages() {
        ArrayList<String> temporary = this.returnMessages;
        this.returnMessages = new ArrayList();

        return temporary;
    }

    public boolean saveChanges(int id, String productName, String availableQuantity, String unitPrice, String reorderLevel) throws SQLException {
        ArrayList<String> returnMessages = this.validateStockItemData(productName, availableQuantity, unitPrice, reorderLevel);

        int numericalAvailableQuantity = 0;
        int numericalReorderLevel = 0;
        double numericalUnitPrice = 0;

        try {
            numericalAvailableQuantity = Integer.parseInt(availableQuantity);
            numericalReorderLevel = Integer.parseInt(reorderLevel);
        } catch (NumberFormatException exception) {
            returnMessages.add("Available quantity and re-order level may only be whole numbers!");
        }

        try {
            numericalUnitPrice = Double.parseDouble(unitPrice);
        } catch (NumberFormatException exception) {
            returnMessages.add("The unit price may only contain numbers and a decimal point!");
        }

        if(returnMessages.isEmpty()) {
            StockItem stockItem = new StockItem(id, productName, numericalAvailableQuantity, numericalUnitPrice, numericalReorderLevel);
            this.stockRepository.updateStockItem(stockItem);

            return true;
        }

        this.returnMessages = returnMessages;

        return false;
    }

    public ArrayList<StockItem> fetchAllStockItems() throws SQLException {
        return this.stockRepository.fetchAllStockItems();
    }

    public void removeStockItem(int id) throws SQLException {
        this.stockRepository.removeStockItem(id);
    }
}

com.HassanAlthaf.StockControlSystem.Stocks.StockItem.java

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

import javafx.beans.property.DoubleProperty;
import javafx.beans.property.IntegerProperty;
import javafx.beans.property.SimpleDoubleProperty;
import javafx.beans.property.SimpleIntegerProperty;
import javafx.beans.property.SimpleStringProperty;
import javafx.beans.property.StringProperty;

public class StockItem {
    private IntegerProperty id;
    private StringProperty productName;
    private IntegerProperty availableQuantity;
    private DoubleProperty unitPrice;
    private IntegerProperty reorderLevel;
    private DoubleProperty totalValue;

    public StockItem(String productName, int availableQuantity, double unitPrice, int reorderLevel) {
        this.id = new SimpleIntegerProperty(0);
        this.productName = new SimpleStringProperty(productName);
        this.availableQuantity = new SimpleIntegerProperty(availableQuantity);
        this.unitPrice = new SimpleDoubleProperty(unitPrice);
        this.reorderLevel = new SimpleIntegerProperty(reorderLevel);
        double totalValue = unitPrice * availableQuantity;
        this.totalValue = new SimpleDoubleProperty(totalValue);
    }

    public StockItem(int id, String productName, int availableQuantity, double unitPrice, int reorderLevel) {
        this.id = new SimpleIntegerProperty(id);
        this.productName = new SimpleStringProperty(productName);
        this.availableQuantity = new SimpleIntegerProperty(availableQuantity);
        this.unitPrice = new SimpleDoubleProperty(unitPrice);
        this.reorderLevel = new SimpleIntegerProperty(reorderLevel);
        double totalValue = unitPrice * availableQuantity;
        this.totalValue = new SimpleDoubleProperty(totalValue);
    }

    public void setID(int id) {
        this.id = new SimpleIntegerProperty(id);
    }

    public int getID() {
        return this.id.get();
    }

    public void setProductName(String productName) {
        this.productName = new SimpleStringProperty(productName);
    }

    public String getProductName() {
        return this.productName.get();
    }

    public void setAvailableQuantity(int availableQuantity) {
        this.availableQuantity = new SimpleIntegerProperty(availableQuantity);
    }

    public int getAvailableQuantity() {
        return this.availableQuantity.get();
    }

    public void setUnitPrice(double unitPrice) {
        this.unitPrice = new SimpleDoubleProperty(unitPrice);
    }

    public double getUnitPrice() {
        return this.unitPrice.get();
    }

    public void setReorderLevel(int reorderLevel) {
        this.reorderLevel = new SimpleIntegerProperty(reorderLevel);
    }

    public int getReorderLevel() {
        return this.reorderLevel.get();
    }

    public String getTotalValue() {
        return String.format("%.2f", this.totalValue.get());
    }
}

com.HassanAlthaf.StockControlSystem.Stocks.StockRepository.java

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

import com.HassanAlthaf.StockControlSystem.Database.DatabaseAdapter;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.ArrayList;

public class StockRepository {
    private final DatabaseAdapter databaseAdapter;

    public StockRepository() {
        this.databaseAdapter = new DatabaseAdapter();
    }

    public void addStockItem(StockItem stockItem) throws SQLException {
        Connection connection = this.databaseAdapter.getConnection();

        PreparedStatement statement = connection.prepareStatement("INSERT INTO \"public\".\"stockItems\" ( \"productName\", \"availableQuantity\", \"unitPrice\", \"reorderLevel\") VALUES(?, ?, ?, ?)");
        statement.setString(1, stockItem.getProductName());
        statement.setInt(2, stockItem.getAvailableQuantity());
        statement.setDouble(3, stockItem.getUnitPrice());
        statement.setInt(4, stockItem.getReorderLevel());

        statement.execute();
        statement.close();
        connection.close();
    }

    public void removeStockItem(int id) throws SQLException {
        Connection connection = this.databaseAdapter.getConnection();

        PreparedStatement statement = connection.prepareStatement("DELETE FROM \"public\".\"stockItems\" WHERE \"id\" = ?");
        statement.setInt(1, id);

        statement.execute();
        statement.close();
        connection.close();
    }

    public void fetchStockItem(int id) {

    }

    public ArrayList<StockItem> fetchAllStockItems() throws SQLException {
        ArrayList<StockItem> stockItems = new ArrayList<>();
        Connection connection = this.databaseAdapter.getConnection();

        PreparedStatement statement = connection.prepareStatement("SELECT * FROM \"public\".\"stockItems\"");
        ResultSet resultSet = statement.executeQuery();
        while(resultSet.next()) {
            StockItem stockItem = new StockItem(
                resultSet.getInt("id"),
                resultSet.getString("productName"),
                resultSet.getInt("availableQuantity"),
                resultSet.getDouble("unitPrice"),
                resultSet.getInt("reorderLevel")
            );

            stockItems.add(stockItem);
        }

        resultSet.close();
        statement.close();
        connection.close();

        return stockItems;
    }

    public boolean doesStockItemExist(int id) throws SQLException {
        Connection connection = this.databaseAdapter.getConnection();

        PreparedStatement statement = connection.prepareStatement("SELECT * FROM \"public\".\"stockItems\" WHERE \"id\" = ?");
        statement.setInt(1, id);
        ResultSet resultSet = statement.executeQuery();

        int count = 0;

        while(resultSet.next()) {
            count++;
        }

        resultSet.close();
        statement.close();
        connection.close();

        if(count == 0) {
            return false;
        } else {
            return true;
        }
    }

    public void updateStockItem(StockItem stockItem) throws SQLException {
        if(this.doesStockItemExist(stockItem.getID())) {
            System.out.println(stockItem.getProductName() + " exists!");
            Connection connection = this.databaseAdapter.getConnection();

            PreparedStatement statement = connection.prepareCall("UPDATE \"public\".\"stockItems\" SET \"productName\" = ?, \"availableQuantity\" = ?, \"unitPrice\" = ?, \"reorderLevel\" = ? WHERE \"id\" = ?");
            statement.setString(1, stockItem.getProductName());
            statement.setInt(2, stockItem.getAvailableQuantity());
            statement.setDouble(3, stockItem.getUnitPrice());
            statement.setInt(4, stockItem.getReorderLevel());
            statement.setInt(5, stockItem.getID());

            statement.execute();
            statement.close();
            connection.close();
        }
    }
}

com.HassanAlthaf.StockControlSystem.Users.User.java

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

import javafx.beans.property.IntegerProperty;
import javafx.beans.property.SimpleIntegerProperty;
import javafx.beans.property.StringProperty;
import javafx.beans.property.SimpleStringProperty;

public class User {
    private IntegerProperty id;
    private StringProperty username;
    private StringProperty password;
    private IntegerProperty rank;

    public User() {
        this.id = new SimpleIntegerProperty(0);
        this.username = new SimpleStringProperty("");
        this.password = new SimpleStringProperty("");
        this.rank = new SimpleIntegerProperty(1);
    }

    public User(int id, String username, String password, int rank) {
        this.id = new SimpleIntegerProperty(id);
        this.username = new SimpleStringProperty(username);
        this.password = new SimpleStringProperty(password);
        this.rank = new SimpleIntegerProperty(rank);
    }

    public User(String username, String password, int rank) {
        this.username = new SimpleStringProperty(username);
        this.password = new SimpleStringProperty(password);
        this.rank = new SimpleIntegerProperty(rank);
    }

    public int getID() {
        return this.id.get();
    }

    public void setID(int id) {
        this.id = new SimpleIntegerProperty(id);
    }

    public String getUsername() {
        return this.username.get();
    }

    public void setUsername(String username) {
        this.username = new SimpleStringProperty(username);
    }

    public String getPassword() {
        return this.password.get();
    }

    public void setPassword(String password) {
        this.password = new SimpleStringProperty(password);
    }

    public int getRank() {
        return this.rank.get();
    }

    public void setRank(int rank) {
        this.rank = new SimpleIntegerProperty(rank);
    }

    private StringProperty SimpleStringProperty(String string) {
        throw new UnsupportedOperationException("Not supported yet.");
    }

    private IntegerProperty SimpleIntegerProperty(int i) {
        throw new UnsupportedOperationException("Not supported yet.");
    }
}

com.HassanAlthaf.StockControlSystem.Users.UserController.java

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

import java.sql.SQLException;
import java.util.ArrayList;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import org.mindrot.jbcrypt.BCrypt;

public class UserController {
    private boolean isUserLoggedIn = false;
    private User loggedInUser;
    private UserRepository userRepository;

    public final static int USER_INVALID_CREDENTIALS = 0;
    public final static int USER_VALID_CREDENTIALS = 1;
    public final static int USER_ACCESS_DENIED = 2;

    public final static int RANK_DISABLED_USER = 0;
    public final static int RANK_REGULAR_USER = 1;
    public final static int RANK_ADMINISTRATOR = 2;

    private ArrayList<String> validationErrors;

    public UserController() {
        this.userRepository = new UserRepository();
    }

    public int validateCredentials(String username, String password) throws SQLException {
        if(this.userRepository.isUsernameTaken(username)) {
            User user = this.userRepository.fetchUserByUsername(username);
            if(BCrypt.checkpw(password, user.getPassword())) {
                if(user.getRank() == UserController.RANK_DISABLED_USER) {
                    return UserController.USER_ACCESS_DENIED;
                }

                return UserController.USER_VALID_CREDENTIALS;
            }
        }

        return UserController.USER_INVALID_CREDENTIALS;
    }

    public int login(String username, String password) throws SQLException {
        int result = this.validateCredentials(username, password);

        if(result == UserController.USER_VALID_CREDENTIALS) {
            this.isUserLoggedIn = true;
            this.loggedInUser = this.userRepository.fetchUserByUsername(username);
        }

        return result;
    }

    public void logout() {
        this.isUserLoggedIn = false;
        this.loggedInUser = new User();
    }

    public User getLoggedInUser() {
        return this.loggedInUser;
    }

    public void updateLoggedInUser() throws SQLException {
        int id = this.loggedInUser.getID();
        this.loggedInUser = this.userRepository.fetchUser(id);
    }

    public boolean userIsDisabled() throws SQLException {
        this.updateLoggedInUser();

        if(this.loggedInUser.getRank() == UserController.RANK_DISABLED_USER) {
            return true;
        }

        return false;
    }

    public boolean isAdmin() {
        if(this.loggedInUser.getRank() == UserController.RANK_ADMINISTRATOR) {
            return true;
        }

        return false;
    }

    public boolean createUser(String username, String password, int rank) throws SQLException {
        this.validationErrors = new ArrayList<>();
        this.validationErrors.addAll(this.validateUsername(username));
        this.validationErrors.addAll(this.validatePassword(password));
        this.validationErrors.addAll(this.validateRank(rank));

        if(this.validationErrors.isEmpty()) {
            password = BCrypt.hashpw(password, BCrypt.gensalt());

            User user = new User(username, password, rank);

            this.userRepository.createUser(user);

            return true;
        }

        return false;
    }

    public ArrayList<String> returnAllValidationErrors() {
        ArrayList<String> returnData = new ArrayList<>();
        returnData.addAll(this.validationErrors);
        this.validationErrors.clear();
        return returnData;
    }

    public ArrayList<String> validateUsername(String username) throws SQLException {
        ArrayList<String> validationErrors = new ArrayList<>();

        if(username.equals("")) {
            validationErrors.add("The username field is required to be filled!");
        }

        if(username.length() < 3 || username.length() > 20) {
            validationErrors.add("The username must be at least 3 characters in length and cannot go over 20.");
        }

        Pattern pattern = Pattern.compile("^[a-zA-Z0-9]*$");
        Matcher matcher = pattern.matcher(username);

        if(!matcher.find()) {
            validationErrors.add("The username may only contain lowercase and uppercase alphabets and numbers.");
        }

        if(this.userRepository.isUsernameTaken(username)) {
            validationErrors.add("This username is already taken! Please try another one!");
        }

        return validationErrors;
    }

    public ArrayList<String> validatePassword(String password) {
        ArrayList<String> validationErrors = new ArrayList<>();

        if(password.equals("")) {
            validationErrors.add("The password field is required to be filled!");
        }

        if(password.length() < 5 || password.length() > 72) {
            validationErrors.add("The password must be at least 5 characters in length and cannot go over 72.");
        }

        return validationErrors;
    }

    public ArrayList<String> validateRank(int rank) {
        ArrayList<String> validationErrors = new ArrayList<>();

        if(rank != UserController.RANK_ADMINISTRATOR && rank != UserController.RANK_DISABLED_USER && rank != UserController.RANK_REGULAR_USER) {
            validationErrors.add("Invalid user rank specified.");
        }

        return validationErrors;
    }

    public ArrayList<User> fetchAllUsers() throws SQLException {
        return this.userRepository.fetchAllUsers();
    }

    public boolean editUser(String username, String password, int rank, User user) throws SQLException {
        this.validationErrors = new ArrayList<>();

        if(!username.equals(user.getUsername())) {
            this.validationErrors.addAll(this.validateUsername(username));
            user.setUsername(username);
        }

        if(!password.equals("")) {
            this.validationErrors.addAll(this.validatePassword(password));
            user.setPassword(BCrypt.hashpw(password, BCrypt.gensalt()));
        }

        if(rank != user.getRank()) {
            this.validationErrors.addAll(this.validateRank(rank));
            user.setRank(rank);
        }

        if(this.validationErrors.isEmpty()) {
            this.userRepository.updateUser(user);

            return true;
        }

        return false;
    }

    public void deleteUser(int id) throws SQLException {
        if(this.userRepository.doesUserExist(id) && this.loggedInUser.getID() != id) {
            this.userRepository.deleteUser(id);
        }
    }
}

com.HassanAlthaf.StockControlSystem.Users.UserRepository.java

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

import com.HassanAlthaf.StockControlSystem.Database.DatabaseAdapter;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.ArrayList;

public class UserRepository {
    private DatabaseAdapter databaseAdapter;

    public UserRepository() {
        this.databaseAdapter = new DatabaseAdapter();
    }

    public void createUser(User user) throws SQLException {
        Connection connection = this.databaseAdapter.getConnection();

        PreparedStatement statement = connection.prepareStatement("INSERT INTO \"public\".\"users\" (\"username\", \"password\", \"rank\") VALUES(?, ?, ?)");
        statement.setString(1, user.getUsername());
        statement.setString(2, user.getPassword());
        statement.setInt(3, user.getRank());
        statement.execute();
        statement.close();
        connection.close();
    }

    public void updateUser(User user) throws SQLException {
        if(this.doesUserExist(user.getID())) {
            Connection connection = this.databaseAdapter.getConnection();

            PreparedStatement statement = connection.prepareStatement("UPDATE \"public\".\"users\" SET \"username\" = ?, \"password\" = ?, \"rank\" = ? WHERE \"id\" = ?");
            statement.setString(1, user.getUsername());
            statement.setString(2, user.getPassword());
            statement.setInt(3, user.getRank());
            statement.setInt(4, user.getID());
            statement.execute();
            statement.close();
            connection.close();
        }
    }

    public boolean doesUserExist(int id) throws SQLException {
        Connection connection = this.databaseAdapter.getConnection();

        PreparedStatement statement = connection.prepareStatement("SELECT COUNT(*) FROM \"public\".\"users\" WHERE \"id\" = ?");
        statement.setInt(1, id);
        ResultSet resultSet = statement.executeQuery();

        resultSet.next();
        int count = resultSet.getInt(1);

        resultSet.close();
        statement.close();
        connection.close();

        if(count == 0) {
            return false;
        }

        return true;
    }

    public boolean isUsernameTaken(String username) throws SQLException {
        Connection connection = this.databaseAdapter.getConnection();

        PreparedStatement statement = connection.prepareStatement("SELECT COUNT(*) FROM \"public\".\"users\" WHERE \"username\" = ?");
        statement.setString(1, username);
        ResultSet resultSet = statement.executeQuery();

        resultSet.next();
        int count = resultSet.getInt(1);

        if(count == 0) {
            return false;
        }

        return true;
    }

    public User fetchUser(int id) throws SQLException {
        Connection connection = this.databaseAdapter.getConnection();

        PreparedStatement statement = connection.prepareStatement("SELECT * FROM \"public\".\"users\" WHERE \"id\" = ?");
        statement.setInt(1, id);
        ResultSet resultSet = statement.executeQuery();

        resultSet.next();

        User user = new User();

        user.setID(id);
        user.setUsername(resultSet.getString("username"));
        user.setPassword(resultSet.getString("password"));
        user.setRank(resultSet.getInt("rank"));

        return user;
    }

    public User fetchUserByUsername(String username) throws SQLException {
        Connection connection = this.databaseAdapter.getConnection();

        PreparedStatement statement = connection.prepareStatement("SELECT * FROM \"public\".\"users\" WHERE \"username\" = ?");
        statement.setString(1, username);
        ResultSet resultSet = statement.executeQuery();

        resultSet.next();

        User user = new User();

        user.setID(resultSet.getInt("id"));
        user.setUsername(username);
        user.setPassword(resultSet.getString("password"));
        user.setRank(resultSet.getInt("rank"));

        return user;
    }

    public ArrayList<User> fetchAllUsers() throws SQLException {
        ArrayList<User> users = new ArrayList<>();

        Connection connection = this.databaseAdapter.getConnection();
        PreparedStatement statement = connection.prepareStatement("SELECT * FROM \"public\".\"users\"");
        ResultSet resultSet = statement.executeQuery();

        while(resultSet.next()) {
            User user = new User();

            user.setID(resultSet.getInt("id"));
            user.setUsername(resultSet.getString("username"));
            user.setPassword(resultSet.getString("password"));
            user.setRank(resultSet.getInt("rank"));

            users.add(user);
        }

        return users;
    }

    public void deleteUser(int id) throws SQLException {
        Connection connection = this.databaseAdapter.getConnection();

        PreparedStatement statement = connection.prepareStatement("DELETE FROM \"public\".\"users\" WHERE \"id\" = ?");
        statement.setInt(1, id);
        statement.execute();
        statement.close();
        connection.close();
    }
}

com.HassanAlthaf.StockControlSystem.View.AddUserDialog.fxml

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

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


<AnchorPane id="AnchorPane" fx:id="addUserDialog" maxHeight="400.0" maxWidth="350.0" minHeight="400.0" minWidth="350.0" prefHeight="400.0" prefWidth="350.0" xmlns:fx="http://javafx.com/fxml/1" xmlns="http://javafx.com/javafx/8" fx:controller="com.HassanAlthaf.StockControlSystem.View.AddUserDialogView">
   <children>
      <Label layoutX="146.0" layoutY="32.0" text="Add User" />
      <TextField fx:id="usernameField" layoutX="169.0" layoutY="73.0" />
      <ChoiceBox fx:id="rankChoiceList" layoutX="169.0" layoutY="148.0" prefHeight="27.0" prefWidth="167.0" />
      <Label layoutX="14.0" layoutY="78.0" text="Username: " />
      <Label layoutX="14.0" layoutY="115.0" text="Password:" />
      <Label layoutX="14.0" layoutY="153.0" text="Rank:" />
      <Button layoutX="293.0" layoutY="359.0" mnemonicParsing="false" onAction="#addUser" text="Add" />
      <Button layoutX="14.0" layoutY="359.0" mnemonicParsing="false" onAction="#closeWindow" text="Close" />
      <Text fx:id="addUserValidationErrors" layoutX="14.0" layoutY="205.0" strokeType="OUTSIDE" strokeWidth="0.0" wrappingWidth="322.0" />
      <TextField fx:id="passwordField" layoutX="169.0" layoutY="110.0" />
   </children>
</AnchorPane>

com.HassanAlthaf.StockControlSystem.View.AddUserDialogView.java

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

import com.HassanAlthaf.StockControlSystem.Users.UserController;
import java.io.IOException;
import java.net.URL;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.ResourceBundle;
import javafx.collections.FXCollections;
import javafx.event.ActionEvent;
import javafx.fxml.FXML;
import javafx.fxml.Initializable;
import javafx.scene.Parent;
import javafx.scene.Scene;
import javafx.scene.control.ChoiceBox;
import javafx.scene.control.TextField;
import javafx.scene.text.Text;
import javafx.stage.Stage;

public class AddUserDialogView implements Initializable {
    private MainView mainView;
    private UserController userController;
    private Stage stage;

    @FXML
    private Parent addUserDialog;

    @FXML
    private Text addUserValidationErrors;

    @FXML
    private TextField usernameField;

    @FXML
    private TextField passwordField;

    @FXML
    private ChoiceBox rankChoiceList;

    public void setMainView(MainView mainView) {
        this.mainView = mainView;
    }

    public void setUserController(UserController userController) {
        this.userController = userController;
    }

    public void show() {
        Scene scene = new Scene(this.addUserDialog);

        this.stage = new Stage();
        this.stage.setScene(scene);
        this.stage.setTitle("Add User");
        this.stage.setResizable(false);

        this.populateChoiceBox();

        this.stage.show();
    }

    public void populateChoiceBox() {
        this.rankChoiceList.setItems(FXCollections.observableArrayList(
                "Disabled Account", "Regular User", "Administrator"
            )
        );
    }

    public void addUser(ActionEvent event) throws SQLException, IOException {
        boolean result = this.userController.createUser(
            this.usernameField.getText(),
            this.passwordField.getText(),
            this.rankChoiceList.getSelectionModel().getSelectedIndex()
        );

        if(result) {
            this.addUserValidationErrors.setText("Successfully created user!");
            this.mainView.populateUsersList();
        } else {
            ArrayList<String> errors = this.userController.returnAllValidationErrors();
            String text = "";

            for(String line : errors) {
                text = text + line + "\n";
            }

            text = text.substring(0, (text.length() - 1));

            this.addUserValidationErrors.setText(text);
        }
    }

    public void closeWindow(ActionEvent event) {
        this.stage.close();
    }

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

    }    

}

com.HassanAlthaf.StockControlSystem.View.EditStockItemDialog.fxml

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

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

<AnchorPane id="AnchorPane" fx:id="editStockItemDialog" maxHeight="400.0" maxWidth="350.0" minHeight="400.0" minWidth="350.0" prefHeight="400.0" prefWidth="350.0" xmlns="http://javafx.com/javafx/8" xmlns:fx="http://javafx.com/fxml/1" fx:controller="com.HassanAlthaf.StockControlSystem.View.EditStockItemDialogView">
   <children>
      <Label layoutX="94.0" layoutY="28.0" text="Editting Stock Item">
         <font>
            <Font name="Lato-Light" size="18.0" />
         </font>
      </Label>
      <Label layoutX="25.0" layoutY="73.0" text="Product Name:" />
      <Label layoutX="25.0" layoutY="110.0" text="Available Quantity:" />
      <TextField fx:id="productNameField" layoutX="158.0" layoutY="68.0" />
      <TextField fx:id="availableQuantityField" layoutX="158.0" layoutY="105.0" />
      <TextField fx:id="unitPriceField" layoutX="158.0" layoutY="142.0" />
      <TextField fx:id="reorderLevelField" layoutX="158.0" layoutY="179.0" />
      <Text layoutX="25.0" layoutY="160.0" strokeType="OUTSIDE" strokeWidth="0.0" text="Unit Price:" />
      <Text layoutX="25.0" layoutY="197.0" strokeType="OUTSIDE" strokeWidth="0.0" text="Reorder Level:" />
      <Button layoutX="277.0" layoutY="350.0" mnemonicParsing="false" onAction="#saveEdit" text="Save" />
      <Button layoutX="25.0" layoutY="350.0" mnemonicParsing="false" onAction="#closeEditting" text="Close" />
      <Text fx:id="errorsArea" layoutX="25.0" layoutY="234.0" lineSpacing="3.0" strokeType="OUTSIDE" strokeWidth="0.0" wrappingWidth="300.0" />
   </children>
</AnchorPane>

com.HassanAlthaf.StockControlSystem.View.EditStockItemDialogView.java

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

import com.HassanAlthaf.StockControlSystem.Stocks.StockController;
import com.HassanAlthaf.StockControlSystem.Stocks.StockItem;
import java.io.IOException;
import java.net.URL;
import java.sql.SQLException;
import java.util.ArrayList;
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.TextField;
import javafx.scene.text.Text;
import javafx.stage.Stage;

public class EditStockItemDialogView implements Initializable {

    private StockItem stockItem;
    private StockController stockController;

    @FXML
    private Parent editStockItemDialog;

    @FXML
    private Text errorsArea;

    @FXML
    private TextField productNameField;

    @FXML
    private TextField availableQuantityField;

    @FXML
    private TextField unitPriceField;

    @FXML
    private TextField reorderLevelField;

    private Stage stage;

    private MainView mainView;

    public void setStockItemObject(StockItem stockItem) {
        this.stockItem = stockItem;
    }

    public void setStockController(StockController stockController) {
        this.stockController = stockController;
    }

    public void setMainView(MainView mainView) {
        this.mainView = mainView;
    }

    public void show() {
        Scene scene  = new Scene(this.editStockItemDialog);

        this.stage = new Stage();

        this.stage.setScene(scene);
        this.stage.setTitle("Edit Stock Item");
        this.stage.setResizable(false);
        this.setData();
        this.stage.show();
    }

    public void setData() {
        this.productNameField.setText(this.stockItem.getProductName());
        this.availableQuantityField.setText(String.valueOf(this.stockItem.getAvailableQuantity()));
        this.unitPriceField.setText(String.valueOf(this.stockItem.getUnitPrice()));
        this.reorderLevelField.setText(String.valueOf(this.stockItem.getReorderLevel()));
    }

    public void closeEditting(ActionEvent event) {
        this.stage.close();
    }

    public void saveEdit(ActionEvent event) throws SQLException, IOException {
        boolean response = this.stockController.saveChanges(
                this.stockItem.getID(),
                this.productNameField.getText(), 
                this.availableQuantityField.getText(), 
                this.unitPriceField.getText(), 
                this.reorderLevelField.getText()
        );

        if(response == false) {
            ArrayList<String> errors = this.stockController.getReturnMessages();
            String text = "";

            for(String line : errors) {
                text = text + line + "\n";
            }

            text = text.substring(0, text.length() - 1);

            this.errorsArea.setText(text);
        } else {
            this.errorsArea.setText("Successfully editted!");
            this.mainView.initializeStats();
            this.mainView.populateStocksList();
        }
    }

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

}

com.HassanAlthaf.StockControlSystem.View.EditUserDialog.fxml

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

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


<AnchorPane id="AnchorPane" fx:id="editUserDialog" maxHeight="400.0" maxWidth="350.0" minHeight="400.0" minWidth="350.0" prefHeight="400.0" prefWidth="350.0" xmlns:fx="http://javafx.com/fxml/1" xmlns="http://javafx.com/javafx/8" fx:controller="com.HassanAlthaf.StockControlSystem.View.EditUserDialogView">
   <children>
      <Label layoutX="147.0" layoutY="32.0" text="Edit User" />
      <TextField fx:id="usernameField" layoutX="169.0" layoutY="82.0" />
      <TextField fx:id="passwordField" layoutX="169.0" layoutY="123.0" />
      <ChoiceBox fx:id="rankListChoiceBox" layoutX="169.0" layoutY="160.0" prefHeight="27.0" prefWidth="167.0" />
      <Label layoutX="14.0" layoutY="87.0" text="Username:" />
      <Label layoutX="14.0" layoutY="128.0" text="Password:" />
      <Label layoutX="14.0" layoutY="165.0" text="Rank:" />
      <Button layoutX="294.0" layoutY="359.0" mnemonicParsing="false" onAction="#editUser" text="Edit" />
      <Button layoutX="14.0" layoutY="359.0" mnemonicParsing="false" onAction="#closeWindow" text="Close" />
      <Text fx:id="messagesArea" layoutX="15.0" layoutY="213.0" strokeType="OUTSIDE" strokeWidth="0.0" wrappingWidth="322.0" />
   </children>
</AnchorPane>

com.HassanAlthaf.StockControlSystem.View.EditUserDialogView.java

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

import com.HassanAlthaf.StockControlSystem.Users.User;
import com.HassanAlthaf.StockControlSystem.Users.UserController;
import java.io.IOException;
import java.net.URL;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.ResourceBundle;
import javafx.collections.FXCollections;
import javafx.event.ActionEvent;
import javafx.fxml.FXML;
import javafx.fxml.Initializable;
import javafx.scene.Parent;
import javafx.scene.Scene;
import javafx.scene.control.ChoiceBox;
import javafx.scene.control.TextField;
import javafx.scene.text.Text;
import javafx.stage.Stage;

public class EditUserDialogView implements Initializable {

    @FXML
    private Parent editUserDialog;

    private MainView mainView;
    private UserController userController;
    private Stage stage;

    private User user;

    @FXML
    private Text messagesArea;

    @FXML
    private TextField usernameField;

    @FXML
    private TextField passwordField;

    @FXML
    private ChoiceBox rankListChoiceBox;

    public void setMainView(MainView mainView) {
        this.mainView = mainView;
    }

    public void setUserController(UserController userController) {
        this.userController = userController;
    }

    public void setUserEntity(User user) {
        this.user = user;
    }

    public void show() {
        Scene scene = new Scene(this.editUserDialog);

        this.stage = new Stage();
        this.stage.setScene(scene);
        this.stage.setTitle("Edit User");
        this.stage.setResizable(false);
        this.populateRankChoiceList();
        this.populateFields();
        this.stage.show();
    }

    public void populateRankChoiceList() {
        this.rankListChoiceBox.setItems(FXCollections.observableArrayList(
                "Disabled Account", "Regular User", "Administrator"
            )
        );
    }

    public void populateFields() {
        this.usernameField.setText(this.user.getUsername());
        this.rankListChoiceBox.getSelectionModel().select(this.user.getRank());
    }

    public void editUser(ActionEvent event) throws SQLException, IOException {
        boolean result = this.userController.editUser(
            this.usernameField.getText(),
            this.passwordField.getText(),
            this.rankListChoiceBox.getSelectionModel().getSelectedIndex(),
            this.user
        );

        if(result) {
            this.messagesArea.setText("Successfully updated user!");
            mainView.populateUsersList();
        } else {
            ArrayList<String> errors = this.userController.returnAllValidationErrors();
            String text = "";

            for(String line : errors) {
                text = text + line + "\n";
            }

            text = text.substring(0, (text.length() - 1));

            this.messagesArea.setText(text);
        }
    }

    public void closeWindow(ActionEvent event) {
        this.stage.close();
    }

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

    }    

}

com.HassanAlthaf.StockControlSystem.View.LoginView.fxml

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

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

<AnchorPane id="AnchorPane" fx:id="loginWindow" maxHeight="400.0" maxWidth="300.0" minHeight="400.0" minWidth="300.0" prefHeight="400.0" prefWidth="300.0" xmlns="http://javafx.com/javafx/8" xmlns:fx="http://javafx.com/fxml/1" fx:controller="com.HassanAlthaf.StockControlSystem.View.LoginWindowView">
   <children>
      <Button layoutX="124.0" layoutY="290.0" mnemonicParsing="false" onAction="#login" text="Login" />
      <TextField fx:id="usernameField" layoutX="67.0" layoutY="165.0" promptText="Username" />
      <Label layoutX="56.0" layoutY="40.0" text="Stock Control System" textFill="#404040">
         <font>
            <Font name="Lato Regular" size="20.0" />
         </font>
      </Label>
      <Label layoutX="63.0" layoutY="350.0" text="Developed by Hassan Althaf" />
      <PasswordField fx:id="passwordField" layoutX="67.0" layoutY="208.0" promptText="Password" />
      <Text fx:id="messageField" fill="#b20000" layoutX="26.0" layoutY="101.0" strokeType="OUTSIDE" strokeWidth="0.0" textAlignment="CENTER" wrappingWidth="250.0">
         <font>
            <Font name="Lato-Light" size="16.0" />
         </font>
      </Text>
   </children>
</AnchorPane>

com.HassanAlthaf.StockControlSystem.View.LoginWindowView.java

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

import com.HassanAlthaf.StockControlSystem.Users.UserController;
import java.net.URL;
import java.util.ResourceBundle;
import javafx.event.ActionEvent;
import javafx.fxml.FXML;
import javafx.fxml.FXMLLoader;
import javafx.fxml.Initializable;
import javafx.scene.Parent;
import javafx.scene.Scene;
import javafx.scene.control.PasswordField;
import javafx.scene.control.TextField;
import javafx.scene.text.Text;
import javafx.stage.Stage;

public class LoginWindowView implements Initializable {

    @FXML
    private Parent loginWindow;

    @FXML
    private TextField usernameField;

    @FXML
    private PasswordField passwordField;

    @FXML
    private Text messageField;

    @FXML
    public void login(ActionEvent event) throws Exception {
        String username = this.usernameField.getText();
        String password = this.passwordField.getText();

        UserController userController = new UserController();

        int result = userController.login(username, password);

        if(result == UserController.USER_VALID_CREDENTIALS) {
            FXMLLoader loader = new FXMLLoader(getClass().getResource("MainWindow.fxml"));
            Parent mainWindow = loader.load();
            MainView mainView = loader.getController();

            mainView.setUserController(userController);
            mainView.show(this.loginWindow);
        } else if (result == UserController.USER_ACCESS_DENIED) {
            this.messageField.setText("Your account has been disabled. Please contact an Administrator for more information.");
        } else if (result == UserController.USER_INVALID_CREDENTIALS) {
            this.messageField.setText("Sorry, but the details you provided are invalid.");
        }
    }

    public void show(Parent mainWindow) {
        Scene scene = new Scene(this.loginWindow);

        Stage stage = new Stage();

        stage.setScene(scene);

        stage.setTitle("Authentication Required");
        stage.setResizable(false);

        stage.show();

        Stage mainWindowStage = (Stage) mainWindow.getScene().getWindow();

        mainWindowStage.close();
    }

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

    }    

}

com.HassanAlthaf.StockControlSystem.View.MainView.java

com.HassanAlthaf.StockControlSystem.View.MainWindow.fxml

\$\endgroup\$
7
  • 8
    \$\begingroup\$ That's an awful lot of code to go through. If you post excessively long code samples, it might happen that no one bothers to read through all the code and you get no answers, but you wouldn't be violating any rules. - it's not because you can, that you should... I'd suggest "theming" your post with a more restricted set of classes (say, classes related to "edit stock item" for example), and make it multiple questions (over a number of days) instead - you'll have more interesting posts, and likely more answers. \$\endgroup\$ Oct 18, 2015 at 15:07
  • \$\begingroup\$ That said, I'll be happy to be proven wrong: your post is well-formatted and reads nicely. Have an upvote :) \$\endgroup\$ Oct 18, 2015 at 15:09
  • \$\begingroup\$ Thank you for your help @Mat'sMug I dont have access to a computer till saturday. I will do it when I get access. Till then I can only hope someone to help me.out. \$\endgroup\$
    – codez
    Oct 18, 2015 at 16:37
  • \$\begingroup\$ @MatsMug is it okay to put themed parts of my app again since I have already received answers right now? \$\endgroup\$
    – codez
    Oct 20, 2015 at 4:28
  • \$\begingroup\$ Bounty set. Looking for more answers. \$\endgroup\$
    – codez
    Oct 24, 2015 at 8:35

3 Answers 3

15
+50
\$\begingroup\$

Start using New IO

The java.io packages have been replaced for a reason. They don't integrate well into what the language tries to become and are at best unwieldy. Instead you should rely on the new IO or nio packages, that work on Paths instead of Strings and have some other advantages.

Loading your properties can be simplified to:

 try {
    Properties properties = new Properties();
    properties.load(Files.newInputStream(Paths.get(PROPERTIES_PATH), 
      StandardOpenOption.READ,
      StandardOpenOption.WRITE));
 // ...

Program against interfaces

It's not really good style to have code like:

private ArrayList<String> returnMessages;

instead you should declare fields as their interface where possible:

private List<String> returnMessages;

Don't expose methods unnecessarily

StockController's validateStockItemData is not used in any class beside the StockController itself. Making it public is a big nono. You're violating the principle of information hiding.

I was too lazy to copy over your project into an IDE, but I'm sure there's a lot of similar methods that don't need to be exposed.

This btw. also relates to the fact that you return an ArrayList (program against interfaces!) from a should-be-private method (information hiding!) to write it into a private field of a class instead of directly operating on the field. Don't overcomplicate it for yourself!

Know when to use Wildcard-Imports

import javafx.beans.property.DoubleProperty;
import javafx.beans.property.IntegerProperty;
import javafx.beans.property.SimpleDoubleProperty;
import javafx.beans.property.SimpleIntegerProperty;
import javafx.beans.property.SimpleStringProperty;
import javafx.beans.property.StringProperty;

That's a lot of text for here and also in your class. Any IDE worth it's salt will hide this, but why make it hard. I personally think that 5 to 7 imports is the maximum you should call in from one package if you can avoid it. Then you can still grab a wildcard-import:

import javafx.beans.property.*;

Stay away from integer flags

Integer flags are a remnant of java from versions before java 5. STOP USING THEM. "State of the art" (read: reasonable) is use of an enum to provide typesafety:

public final static int USER_INVALID_CREDENTIALS = 0;
public final static int USER_VALID_CREDENTIALS = 1;
public final static int USER_ACCESS_DENIED = 2;

public final static int RANK_DISABLED_USER = 0;
public final static int RANK_REGULAR_USER = 1;
public final static int RANK_ADMINISTRATOR = 2;

This irresponsible fallback to the past is replaced by:

public enum LoginResult {
    ACCESS_DENIED, INVALID_CREDENTIALS, SUCCESS
}

public enum Rank {
    DISABLED, REGULAR, ADMINISTRATOR
}

Did I mention you can use Enums for switch-statements much easier than integer results?

switch (result) {
    case SUCCESS:
        // show main window
         break;
    case ACCESS_DENIED:
        // account disabled
        break;
    case INVALID_CREDENTIALS:
        // error message
        break;
}

Other notes

  • Precompile patterns into a constant and don't recompile them each run. Patterns are inherently reusable. Don't make your validation routine jump through hoops.
  • Consider using a database access framework (JOOQ, hibernate, ...) instead of writing the access layer yourself.
  • Manual error checking by asking the Controller for messages is so C / C++ style. Use exceptions for error messages where applicable.
  • doesUserExist is probably better off as simply exists or maybe even userExists
\$\endgroup\$
5
  • \$\begingroup\$ Which framework do you recommend? Also how do I precompile patterns? \$\endgroup\$
    – codez
    Oct 20, 2015 at 4:24
  • \$\begingroup\$ @HassanAlthaf Pattern.compile(String). \$\endgroup\$
    – h.j.k.
    Oct 27, 2015 at 2:03
  • \$\begingroup\$ Thank you for everything, it really has cleaned my code a lot. I still have only a bit more to implement according to your tips. \$\endgroup\$
    – codez
    Oct 29, 2015 at 14:33
  • \$\begingroup\$ Regarding using wildcard imports: OP is probably using an IDE (like most of us coding in Java) which automatically manages import statements. I don't see a need for a programmer to do this manually. \$\endgroup\$
    – itchimus
    Nov 2, 2015 at 8:53
  • \$\begingroup\$ Yes I use NetBeans. But planning to be more fluent by coding on notepad or sth. \$\endgroup\$
    – codez
    Nov 2, 2015 at 10:22
11
\$\begingroup\$

Firstly, I'm not familiar with the JavaFX nor PostgreSQL technologies, so my comments will be aimed more at Java-specific code practices. See also the answers by Vogel612 and Antot.

Use "try-with-resources"

Java makes memory management simpler, because it is a garbage-collected language. However, main memory is not the only resource your program will use. Other resources are, for example:

  • File handles
  • Database connections

Your program uses both. If your program throws an Exception, it's possible for the resource to be left open. For instance, in StockRepository.java, what happens when statement.execute throws an Exception? connection is not closed, wasting a resource. Your RDBMS likely has a limit on the number of open connections, and since you haven't closed your connection, you have leaked this resource. If this happens repeatedly, it will result in your RDBMS refusing new connections.

Every time you have an AutoCloseable resource, use "try-with-resources". The language guarantees that every resource will be closed when the block exits. For instance, to implement StockRepository.addStockItem:

public void addStockItem(StockItem stockItem) throws SQLException {
    try( Connection connection = this.databaseAdapter.getConnection();
         PreparedStatement statement = connection.prepareStatement(
             "INSERT INTO \"public\".\"stockItems\" ( \"productName\", \"availableQuantity\", \"unitPrice\", \"reorderLevel\") VALUES(?, ?, ?, ?)"); )
    {
        statement.setString(1, stockItem.getProductName());
        statement.setInt(2, stockItem.getAvailableQuantity());
        statement.setDouble(3, stockItem.getUnitPrice());
        statement.setInt(4, stockItem.getReorderLevel());

        statement.execute();
    }
}

Other methods you should look to do the same thing:

  • DatabaseAdapter.fetchAndSetConnectionDetails (File, FileReader)
  • Every method that uses a Connection, Statement, and ResultSet. (there are a number of them, so I won't point them all out)

Don't retrieve more from the database than you have to

In StockRepository.doesStockItemExist, all you are interested in is whether a row exists in the stockItems table. Instead of selecting every column and then counting the rows, you can run the query:

SELECT COUNT(1) FROM "public"."stockItems" WHERE "id" = ?

Either zero or one row will be returned, and this is easy to check in the Java code, as well as being more efficient. (less data to transfer from your database to the application)

public boolean doesStockItemExist(int id) throws SQLException {
    try( Connection connection = this.databaseAdapter.getConnection();
         PreparedStatement statement = connection.prepareStatement(
            "SELECT COUNT(1) FROM \"public\".\"stockItems\" WHERE \"id\" = ?" ); )
    {
        statement.setInt(1, id);            
        try ( ResultSet resultSet = statement.executeQuery(); )
        {
            return resultSet.next();
        }
    }
}

Don't check the existence of a database row if you aren't going to lock it

In StockRepository.updateStockItem you check to see whether an item exists prior to updating it. However, you haven't locked the row in the database. This means that the row might be deleted in the database by the time you try to update it. You've printed a statement to say the row exists, which might not be true. This might not seem significant for your simple example, but could have greater ramifications for a more complex application. If you want to know the number of rows affected by an UPDATE statement, use Statement.getUpdateCount.

public void updateStockItem(StockItem stockItem) throws SQLException {
    try ( Connection connection = this.databaseAdapter.getConnection();
          PreparedStatement statement = connection.prepareCall(
              "UPDATE \"public\".\"stockItems\" " +
              "SET \"productName\" = ?, " +
              "\"availableQuantity\" = ?, " +
              "\"unitPrice\" = ?, " +
              "\"reorderLevel\" = ? " +
              "WHERE \"id\" = ?"); )
    {
        statement.setString(1, stockItem.getProductName());
        statement.setInt(2, stockItem.getAvailableQuantity());
        statement.setDouble(3, stockItem.getUnitPrice());
        statement.setInt(4, stockItem.getReorderLevel());
        statement.setInt(5, stockItem.getID());

        statement.execute();
        if(statement.getUpdateCount() > 0) {
            System.out.println(stockItem.getProductName() + " exists!");
        }
    }
}

This problem is also prevalent in UserRepository. In several methods, you SELECT the user from the database to check whether they exist, and then do a subsequent SELECT with the column you are interested in, under the assumption that the row will exist. Don't do this; simply SELECT all the columns you need a single time.

Separate concerns

Your UserController class appears to have two separate concerns:

  • determining/changing who is the currently logged-in user, and what their rights are
  • user-related DAO functions

Split these into two separate classes.

Don't require a separate call to retrieve error messages

In UserController.createUser, the caller is required to first check the boolean return value to see whether there were any validation errors, and then make a separate call to returnAllValidationErrors to get the errors if the boolean value indicates there were any. This is bad for two reasons:

  • It makes it more difficult to use the class, since one has to make two method calls where one would suffice. The return value of createUser could be the validation errors.
  • It makes the class non-thread-safe.

Things you are doing right

Immutable objects

Immutable objects have a number of advantages: thread-safety, ease of understanding, etc. You've made StockRepository immutable. Aim to do this wherever possible.

Parameters for SQL statements

The importance of this can't be overstated. SQL injection is one of the biggest causes of security vulnerabilites. It is number 1 on the OWASP Top Ten. Use PreparedStatement.setString et al. (or the equivalent, if you are using a framework) everywhere unless you really know what you are doing (there are specific use-cases for embedding literals in SQL statements)

Salted hash for passwords

You are using bcrypt as your password hashing algorithm. Salted hash algorithms are currently considered to be the best choice for password hashing algorithms as they defend effectively against rainbow table attacks. Read here for an excellent, in-depth analysis of password hashing in the modern day.

Validating data (at all)

Really. Laziness is the source of many a vulnerability. Always go to the effort of validating data from external sources.

\$\endgroup\$
7
  • \$\begingroup\$ I'm impressed! However, I am having difficulties with understanding try-with-resources \$\endgroup\$
    – codez
    Oct 29, 2015 at 12:31
  • \$\begingroup\$ @HassanAlthaf explaining every detail of try-with-resources is difficult to explain in a comment. I'd recommend reading the documentation. (docs.oracle.com/javase/tutorial/essential/exceptions/…) However, simply, all variables you declare in the try must be AutoCloseable and Java guarantees that every one will be closed (i.e. .close() is called) when the block is complete, whether an Exception is thrown or not. "Should I use try-with-resources?" My general rule is you should always use try-with-resources. It makes code easier to read and less bug-prone. \$\endgroup\$
    – itchimus
    Oct 29, 2015 at 12:43
  • \$\begingroup\$ how can I make them autocloseable? \$\endgroup\$
    – codez
    Oct 29, 2015 at 13:03
  • \$\begingroup\$ AutoCloseable is a Java interface. (docs.oracle.com/javase/8/docs/api/java/lang/AutoCloseable.html) Connection, Statement, File etc. already implement this interface. You don't need to do anything. \$\endgroup\$
    – itchimus
    Oct 29, 2015 at 13:07
  • \$\begingroup\$ so all I have to do is, open them in try-catch blocks to achieve what you're saying, yeah? \$\endgroup\$
    – codez
    Oct 29, 2015 at 13:19
10
\$\begingroup\$

I totally agree with Vogel612's observations.

Since there is a lot of code for this review, I'd like to add a couple of items.

DatabaseAdapter Class

In getConnnection() method, there are three issues, two of them are quite serious:

  1. There is a careless call of System.exit(0). I'll just recall a sentence from the good old Hardcore Java about it:

If you use System.exit() at all, your best bet is to use it only in the main method. If you ever write a library and embed System.exit() into it, you probably deserve any resulting physical violence.

  1. This class contains two catch(Exception ex) blocks. Exception is too generic. Dedicated per type catch blocks or a multi-catch block for expected exceptions, like catch (IOException | ClassNotFoundException ex), should be used.

  2. Since JDBC4, there is no more need to call Class.forName(); in order to load the driver. DriverManager should be able to cope with it.

Beans

Bean objects like StokItem and User contain private fields that use property wrappers from javafx.beans.property package. This creates a tight coupling with the package for such simple data structures, without any particular need and adds unnecessary complexity. They should be replaced with basic types (SimpleStringProperty -> String, IntegerProperty -> int etc).

\$\endgroup\$
1
  • \$\begingroup\$ I did Bean properties for TableView. \$\endgroup\$
    – codez
    Oct 19, 2015 at 10:11

Your Answer

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

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