2
\$\begingroup\$

I am developing an ERP system, called 'TeleMart' as a practice at Campus. I have noticed that one of my JavaFX Controller's have become quite complex and large, and most importantly, quite confusing. I have to write really large variable names for page elements, etc. Although I have a quite large code base, I want only this one file reviewed mainly, but review for other files would help as well.

Link to the GitHub repository.

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 com.hassanalthaf.telemart.customers.Customer;
import com.hassanalthaf.telemart.customers.CustomerController;
import com.hassanalthaf.telemart.inventory.ProductController;
import com.hassanalthaf.telemart.inventory.Product;
import com.hassanalthaf.telemart.orders.Order;
import com.hassanalthaf.telemart.orders.OrderController;
import com.hassanalthaf.telemart.orders.OrderItem;
import com.hassanalthaf.telemart.orders.OrderState;
import com.hassanalthaf.telemart.users.UserRanks;
import com.hassanalthaf.telemart.users.UserState;
import java.io.IOException;
import java.net.URL;
import java.util.ResourceBundle;
import javafx.collections.ObservableList;
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.Button;
import javafx.scene.control.CheckBox;
import javafx.scene.control.Label;
import javafx.scene.control.Menu;
import javafx.scene.control.MenuItem;
import javafx.scene.control.TableView;
import javafx.scene.control.TextArea;
import javafx.scene.control.TextField;
import javafx.scene.input.MouseEvent;
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 viewInventory;

    @FXML
    private AnchorPane addCustomer;

    @FXML
    private AnchorPane addInventory;

    @FXML
    private AnchorPane manageCustomers;

    @FXML
    private AnchorPane addOrder;

    @FXML
    private AnchorPane manageOrders;

    @FXML
    private TableView productTableView;

    @FXML
    private Menu userMenu;

    @FXML
    private TextField addCustomerNicNumber;

    @FXML
    private TextField addCustomerName;

    @FXML
    private TextField addCustomerContactNumber;

    @FXML
    private TextField addCustomerAddress;

    @FXML
    private TextField addCustomerEmail;

    @FXML
    private CheckBox addCustomerMembership;

    @FXML
    private Label addCustomerSuccess;

    @FXML
    private Label addCustomerErrors;

    @FXML
    private Label addInventorySuccess;

    @FXML
    private Label addInventoryErrors;

    @FXML
    private TextField addInventoryBrand;

    @FXML
    private TextField addInventoryModel;

    @FXML
    private TextField addInventoryColour;

    @FXML
    private TextField addInventoryUnitPrice;

    @FXML
    private TextField addInventoryAvailableQuantity;

    @FXML
    private TextArea addInventorySpecifications;

    @FXML
    private TableView customersTableView;

    @FXML
    private Label addOrdersSuccessBox;

    @FXML
    private Label addOrdersErrorsBox;

    @FXML
    private TextField addOrdersQuantity;

    @FXML
    private TableView addOrdersTableView;

    @FXML
    private Button addOrdersSelectedCustomer;

    @FXML
    private Label totalBillValue;

    @FXML
    private Label discount;

    @FXML
    private TableView manageOrdersTableView;

    private CustomerController customerController;
    private ProductController productController;
    private OrderController orderController;
    private AnchorPane currentPage;
    private UserState userState;
    private OrderState orderState;


    private void changePage(AnchorPane page, int[] allowedRanks) {
        boolean allowed = false;

        for (int rank : allowedRanks) {
            if (rank == this.userState.getUser().getRank()) {
                allowed = true;
            }
        }

        if (allowed) {
            this.currentPage.setOpacity(0);
            this.currentPage = page;
            this.currentPage.toFront();
            this.currentPage.setOpacity(1);
        }
    }

    public void populateProductsTable() {
        ObservableList<Product> products = this.productTableView.getItems();
        products.clear();
        products.addAll(this.productController.fetchAllProducts());
    }

    public void populateCustomersTable() {
        ObservableList<Customer> customers = this.customersTableView.getItems();
        customers.clear();
        customers.addAll(this.customerController.fetchAllCustomers());
    }

    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();

        if (this.userState.getUser() != null) {
            switch (id) {
                case "homeMenuItem":
                    this.changePage(this.home, new int[]{UserRanks.CASHIER.getValue(), UserRanks.SALES_EXECUTIVE.getValue(), UserRanks.MANAGER.getValue(), UserRanks.ADMINISTRATOR.getValue()});
                    break;
                case "viewInventoryMenuItem":
                    this.changePage(this.viewInventory, new int[]{UserRanks.CASHIER.getValue(), UserRanks.SALES_EXECUTIVE.getValue(), UserRanks.MANAGER.getValue(), UserRanks.ADMINISTRATOR.getValue()});
                    this.populateProductsTable();
                    break;
                case "addCustomerMenuItem":
                    this.changePage(this.addCustomer, new int[]{UserRanks.CASHIER.getValue(), UserRanks.MANAGER.getValue(), UserRanks.ADMINISTRATOR.getValue()});
                    break;
                case "addInventoryMenuItem":
                    this.changePage(this.addInventory, new int[]{UserRanks.MANAGER.getValue(), UserRanks.ADMINISTRATOR.getValue()});
                    break;
                case "manageCustomersMenuItem":
                    this.changePage(this.manageCustomers, new int[]{UserRanks.MANAGER.getValue(), UserRanks.ADMINISTRATOR.getValue()});
                    this.populateCustomersTable();
                    break;
                case "addOrderMenuItem":
                    this.changePage(this.addOrder, new int[]{UserRanks.CASHIER.getValue(), UserRanks.MANAGER.getValue(), UserRanks.ADMINISTRATOR.getValue()});
                    break;
                case "manageOrdersMenuItem":
                    this.changePage(this.manageOrders, new int[]{UserRanks.SALES_EXECUTIVE.getValue(), UserRanks.MANAGER.getValue(), UserRanks.ADMINISTRATOR.getValue()});
                    this.populateManageOrdersTable();
                    break;
                default:
                    this.changePage(this.home, new int[]{UserRanks.CASHIER.getValue(), UserRanks.SALES_EXECUTIVE.getValue(), UserRanks.MANAGER.getValue(), UserRanks.ADMINISTRATOR.getValue()});
                    break;
            }
        }
    }

    @FXML
    private void addCustomer(MouseEvent event) {
        this.addCustomerErrors.setOpacity(0);
        this.addCustomerSuccess.setOpacity(0);

        String nicNumber = this.addCustomerNicNumber.getText();
        String name = this.addCustomerName.getText();

        int contactNumber;

        try {
            contactNumber = Integer.parseInt(this.addCustomerContactNumber.getText());
        } catch (Exception exception) {
            contactNumber = 0;
        }

        String address = this.addCustomerAddress.getText();
        String email = this.addCustomerEmail.getText();
        boolean membership = this.addCustomerMembership.isSelected();

        try {
            this.customerController.addNewCustomer(nicNumber, membership, name, contactNumber, address, email);
            this.addCustomerSuccess.setText("Successfully stored customer details!");
            this.addCustomerSuccess.setOpacity(1);
        } catch (Exception exception) {
            this.addCustomerErrors.setText(exception.getMessage());
            this.addCustomerErrors.setOpacity(1);
        }
    }

    @FXML
    private void addInventory(MouseEvent event) {
        this.addInventorySuccess.setOpacity(0);
        this.addInventoryErrors.setOpacity(0);

        String brand = this.addInventoryBrand.getText();
        String model = this.addInventoryModel.getText();
        String colour = this.addInventoryColour.getText();

        double unitPrice;

        try {
            unitPrice = Double.parseDouble(this.addInventoryUnitPrice.getText());
        } catch (Exception exception) {
            unitPrice = 0;
        }

        int availableQuantity;

        try {
            availableQuantity = Integer.parseInt(this.addInventoryAvailableQuantity.getText());
        } catch (Exception exception) {
            availableQuantity = 0;
        }

        String specifications = this.addInventorySpecifications.getText();

        try {
            this.productController.addNewProduct(brand, model, colour, unitPrice, availableQuantity, specifications);
            this.addInventorySuccess.setText("Successfully stored new inventory details!");
            this.addInventorySuccess.setOpacity(1);
        } catch (Exception exception) {
            this.addInventoryErrors.setText(exception.getMessage());
            this.addInventoryErrors.setOpacity(1);
        }
    }

    @FXML
    private void inventoryTableRefresh(MouseEvent event) {
        this.populateProductsTable();
    }

    private Product getSelectedProduct() {
        Product product = (Product)this.productTableView.getSelectionModel().getSelectedItem();
        return product;
    }

    @FXML
    private void inventoryTableViewDetails(MouseEvent event) throws Exception {
        if (this.productTableView.getSelectionModel().getSelectedItem() != null) {
            FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("/com/hassanalthaf/telemart/views/ViewInventoryDetailsView.fxml"));
            Parent viewInventoryDetails = fxmlLoader.load();
            ViewInventoryDetailsViewModel viewInvetoryDetailsViewModel = fxmlLoader.getController();

            viewInvetoryDetailsViewModel.show(this.getSelectedProduct());
        }
    }

    @FXML
    private void inventoryTableUpdate(MouseEvent event) throws Exception {
        if (this.productTableView.getSelectionModel().getSelectedItem() != null) {
            FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("/com/hassanalthaf/telemart/views/UpdateInventoryDetailsView.fxml"));
            Parent updateInventoryDetails = fxmlLoader.load();
            UpdateInventoryDetailsViewModel updateInventoryDetailsViewModel = fxmlLoader.getController();

            updateInventoryDetailsViewModel.show(this.getSelectedProduct(), this);
        }
    }

    @FXML
    private void inventoryTableDelete(MouseEvent event) {
        if (this.productTableView.getSelectionModel().getSelectedItem() != null) {
            int id = this.getSelectedProduct().getId();

            this.productController.deleteProduct(id);
            this.populateProductsTable();
        }
    }

    private Customer getSelectedCustomer() {
        Customer customer = (Customer)this.customersTableView.getSelectionModel().getSelectedItem();
        return customer;
    }

    @FXML
    private void customerTableRefresh(MouseEvent event) {
        this.populateCustomersTable();
    }

    @FXML
    private void customerTableUpdate(MouseEvent event) throws Exception {
        if (this.customersTableView.getSelectionModel().getSelectedItem() != null) {
            FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("/com/hassanalthaf/telemart/views/UpdateCustomerDetails.fxml"));
            Parent updateCustomerDetails = fxmlLoader.load();
            UpdateCustomerDetailsViewModel updateCustomerDetailsViewModel = fxmlLoader.getController();
            updateCustomerDetailsViewModel.show(this.getSelectedCustomer(), this.customerController, this);
        }
    }

    @FXML
    private void customerTableView(MouseEvent event) throws Exception {
        if (this.customersTableView.getSelectionModel().getSelectedItem() != null) {
            FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("/com/hassanalthaf/telemart/views/ViewCustomer.fxml"));
            Parent viewCustomer = fxmlLoader.load();
            ViewCustomerViewModel viewCustomerViewModel = fxmlLoader.getController();
            viewCustomerViewModel.show(this.getSelectedCustomer());
        }
    }

    @FXML
    private void customerTableDelete(MouseEvent event) {
        if(this.customersTableView.getSelectionModel().getSelectedItem() != null) {
            int id = this.getSelectedCustomer().getId();

            this.customerController.deleteCustomer(id);
            this.populateCustomersTable();
        }
    }

    @FXML
    private void addOrdersSelectProduct(MouseEvent event) throws IOException {
        FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("/com/hassanalthaf/telemart/views/SelectProduct.fxml"));
        Parent selectProduct = fxmlLoader.load();
        SelectProductViewModel selectProductViewModel = fxmlLoader.getController();
        selectProductViewModel.show(this, this.orderState.getSelectedProducts());
    }

    public void selectProduct(Product product) {
        this.orderState.select(product);
        this.addOrdersSuccess("Product successfully selected!");
    }

    private void addOrdersSuccess(String message) {
        this.addOrdersErrorsBox.setOpacity(0);
        this.addOrdersSuccessBox.setText(message);
        this.addOrdersSuccessBox.setOpacity(1);
    }

    private void addOrdersError(String message) {
        this.addOrdersSuccessBox.setOpacity(0);
        this.addOrdersErrorsBox.setText(message);
        this.addOrdersErrorsBox.setOpacity(1);
    }

    private OrderItem getSelectedOrderItem() {
        return (OrderItem)this.addOrdersTableView.getSelectionModel().getSelectedItem();
    }

    private void refreshAddOrderItemsTable() {
        ObservableList<OrderItem> orderItems = this.addOrdersTableView.getItems();
        orderItems.clear();
        orderItems.addAll(this.orderState.getOrderItems());
    }

    private void updateTotalBillValue(double value) {

    }

    @FXML
    private void addOrderItem(MouseEvent event) {
        boolean valid = true;

        if (this.orderState.isProductSelected()) {
            int quantity;

            try {
                quantity = Integer.parseInt(this.addOrdersQuantity.getText());
            } catch (Exception exception) {
                quantity = 0;
                valid = false;
            }

            try {
                this.orderState.setQuantity(quantity);
            } catch (Exception exception) {
                this.addOrdersError(exception.getMessage());
                valid = false;
            }

            if (valid) {
                OrderItem orderItem = this.orderState.getOrderItem();
                orderItem.setUnitPrice(orderItem.getProduct().getUnitPrice());
                this.orderState.setOrderItem(orderItem);
                this.orderState.saveOrderItem();
                this.refreshBillValues();
                this.refreshAddOrderItemsTable();
                this.addOrdersSuccess("Successfully added product!");
                this.addOrdersQuantity.setText("");
            }
        } else {
            this.addOrdersError("Please select a product!");
        }
    }

    @FXML
    private void removeOrderItem(MouseEvent event) {
        if (this.addOrdersTableView.getSelectionModel().getSelectedItem() != null) {
            this.orderState.removeOrderItem(this.getSelectedOrderItem());
            this.refreshBillValues();
            this.refreshAddOrderItemsTable();
            this.addOrdersSuccess("Successfully removed product!");
        } else {
            this.addOrdersError("Select an item to remove from table.");
        }
    }

    public void setOrderCustomer(Customer customer) {
        this.orderState.setCustomer(customer, false);
        this.addOrdersSelectedCustomer.setText(customer.getNicNumber());
        this.addOrdersSelectedCustomer.setDisable(false);
        this.refreshBillValues();
        this.addOrdersSuccess("Successfully selected customer!");
    }

    @FXML
    private void addOrdersSelectCustomer(MouseEvent event) throws IOException {
        FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("/com/hassanalthaf/telemart/views/SelectCustomer.fxml"));
        Parent selectCustomer = fxmlLoader.load();
        SelectCustomerViewModel selectCustomerViewModel = fxmlLoader.getController();

        selectCustomerViewModel.show(this);
    }

    @FXML
    private void addOrdersViewSelectedCustomer(MouseEvent event) throws IOException {
        FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("/com/hassanalthaf/telemart/views/ViewCustomer.fxml"));
        Parent viewCustomer = fxmlLoader.load();
        ViewCustomerViewModel viewCustomerViewModel = fxmlLoader.getController();

        viewCustomerViewModel.show(this.orderState.getSelectedCustomer());
    }

    private void resetSelectedButton() {
        this.addOrdersSelectedCustomer.setDisable(true);
        this.addOrdersSelectedCustomer.setText("None");
    }

    @FXML
    private void addOrdersUnselectCustomer(MouseEvent event) {
        this.orderState.setCustomer((Customer)null, true);
        this.resetSelectedButton();
        this.refreshBillValues();
        this.addOrdersSuccess("Successfully un-selected customer!");
    }

    @FXML
    private void addOrdersReset(MouseEvent event) {
        this.orderState = new OrderState();
        this.resetSelectedButton();
        this.addOrdersQuantity.setText("");
        this.addOrdersTableView.getItems().clear();
        this.addOrdersSuccess("Order has been successfully reset!");
    }

    @FXML
    private void submitOrder(MouseEvent event) {
        if (!this.orderState.isCustomerSelected()) {
            this.addOrdersError("You need to select a customer!");
            return;
        }

        try {
            this.orderController.save(this.orderState.getOrder(), this.userState.getUser().getId());
            this.addOrdersSuccess("Successfully created order!");
        } catch (Exception exception) {
            exception.printStackTrace();
            this.addOrdersError(exception.getMessage());
        }
    }

    public void refreshBillValues() {
        this.totalBillValue.setText(String.format("%.2f", this.orderState.getBillValue()));
        this.discount.setText(String.format("%.2f", this.orderState.getDiscountedAmount()));
    }

    private void populateManageOrdersTable() {
        try {
            ObservableList<Order> orders = this.manageOrdersTableView.getItems();
            orders.clear();
            orders.addAll(this.orderController.fetchAll());
        } catch (Exception exception) {
            exception.printStackTrace();
        }
    }

    @FXML
    private void manageOrderView(MouseEvent event) throws Exception {
        if (this.manageOrdersTableView.getSelectionModel().getSelectedItem() != null) {
            Order order = (Order)this.manageOrdersTableView.getSelectionModel().getSelectedItem();

            FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("/com/hassanalthaf/telemart/views/ViewOrder.fxml"));
            Parent viewOrder = fxmlLoader.load();
            ViewOrderViewModel viewOrderViewModel = fxmlLoader.getController();

            viewOrderViewModel.show(order);
        }
    }

    @FXML
    private void undoOrder(MouseEvent event) throws Exception {
        if (this.manageOrdersTableView.getSelectionModel().getSelectedItem() != null) {
            Order order = (Order)this.manageOrdersTableView.getSelectionModel().getSelectedItem();

            order = this.orderController.fetch(order.getId());

            this.orderController.delete(order);

            this.populateManageOrdersTable();
        }
    }

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

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

        this.userMenu.setText(userState.getUser().getUsername());

        stage.show();

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

        this.userState = userState;
        this.orderState = new OrderState();

        this.customerController = new CustomerController();
        this.productController = new ProductController();
        this.orderController = new OrderController();
    }

    @Override
    public void initialize(URL url, ResourceBundle rb) {
        this.currentPage = this.home;
    }
}

This is my first ever Java project which is of this size, therefore, review in all topics would be helpful, but my main focus is on:

  • Making the above code more neat and maintainable.
  • How I could improve not showing pages to un-authorised people than what I am doing currently.
  • Any better way of doing what I am trying to do.
  • Any bad practices.
  • Any conceptual or conventional flaw in my code.
  • Is my code having issues with being SOLID? How can I rectify that?
\$\endgroup\$
3
\$\begingroup\$

TL;DR : move your application logic out of the UI component(s).

Tactics

    boolean allowed = false;

    for (int rank : allowedRanks) {
        if (rank == this.userState.getUser().getRank()) {
            allowed = true;
        }
    }

Don't type that out every time, think about what you are doing, and write a function to do it. With a bit of thought, you should be able to see that this code is really

 // Warning: pseudo code ahead
 boolean allowed = set(allowedRanks).contains(rank);

When you get that far, you might then realize that int is the wrong way to track UserRank -- what you really want is an Enum.

 EnumSet<UserRank> allowedRanks = EnumSet.of(...);
 boolean allowed = allowedRanks.contains(user.getRank());

A guideline: a function should perform coordination, calculation, or creation. If you see more than one of those things happening together, that's a suggestion that you are doing too much in one place.

BEFORE

private void changePage(AnchorPane page, int[] allowedRanks) {
    boolean allowed = false;

    for (int rank : allowedRanks) {
        if (rank == this.userState.getUser().getRank()) {
            allowed = true;
        }
    }

    if (allowed) {
        this.currentPage.setOpacity(0);
        this.currentPage = page;
        this.currentPage.toFront();
        this.currentPage.setOpacity(1);
    }
}

AFTER

private void changePage(...) {
    // pure coordination, do the calculation elsewhere
    if (allowedToView(userState.getUser, allowedRanks)) {
        turnPage(currentPage, page);
    }
}

Strategy

One area where you are heading toward trouble is the separation of your UI, and your logic. An example

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();

    if (this.userState.getUser() != null) {
        switch (id) {
            case "homeMenuItem":
                this.changePage(this.home, new int[]{UserRanks.CASHIER.getValue(), UserRanks.SALES_EXECUTIVE.getValue(), UserRanks.MANAGER.getValue(), UserRanks.ADMINISTRATOR.getValue()});
                break;
    // ...

ActionEvent, MenuItem, AnchorPane are all part of your UI framework. User, UserRanks are part of your model. You shouldn't be able to see that code in the same place.

Thought experiment: imagine that you wanted to have a web version of this program. Which parts would get rid of? Well, you probably wouldn't need the JavaFX classes any longer, so all that can go. You probably would need your Users, and UserRanks, and the rules that decide who can see bits of information. That's the big hint that there are two pieces that should be separated -- the UI isn't the program, it's how the human operator talks to the program.

Thought experiment: how would you write automated tests? As a rule, trying to write good tests for the elements in the UI framework is a pain. But making sure that page transitions work should be testable. Answer: three components. The outer component is the expression of your UI in the language of the framework you are using; you make it as thin as you possibly can. The second component is the UI logic, written in an agnostic form. All of the UI decision making is happening in the second component; the first component is just translation.

Heuristic that you have it right -- the first component looks really trivial.

These two separate components together make up your UI; when you are thinking about the hypothetical web version, they both go away.

Second component talks to the third component, which is where all of the application rules live. Second component and third component send messages back and for about what the user tried to do, and what to show.

The sequence of messages might look like

  • 1 to 2: MouseEvent(TurnPageButton.clicked)
  • 2 to 3: PageTurnRequest(from:4 to:5)
  • 3 to 2: HidePage(4)
  • 2 to 1: page4.opacity = 0
  • 3 to 2: ShowPage(5)
  • 2 to 1: page5.toFront
  • 2 to 1: page5.opacity = 1

Things to check

  • The code in component 1 is so simple it can't possibly be broken
  • All of the constants that JavaFX uses (like OPACITY_HIDE=0, OPACITY_SHOW=1, and so on) live in component 2
  • Component 2 has in memory every significant property of every object in the UI. In other words, you should be able to look at component 2 in the debugger, and see the opacity of every single page.
  • Your automated test framework can pretend to be component 1, trading message back and forth with component 2 to make sure that the UI works. You shouldn't need to duplicate any code to make this happen.
  • Your automated test framework can pretend to be component 2, trading message back and forth with component 3 to make sure the application behind the UI works.

Other

    try {
        availableQuantity = Integer.parseInt(this.addInventoryAvailableQuantity.getText());
    } catch (Exception exception) {
        availableQuantity = 0;
    }

User input validation is its own thing; it belongs in the app, not in the UI. There's often an "anti corruption" component that is responsible for taking unvalidated data, checking that it conforms to the rules governing its type, and then expressing the data as a immutable value type, so that the rest of the program knows that everything is correctly typed.

ValidQuantity availableQuantity = ValidQuantity.parse(userInput);

Either availableQuantity holds a valid quantity value (can quantity be negative?), or it throws an exception (IllegalArgumentException is the usual choice). Typically, in a desk top application where the user can fix the problem, you would send some sort of message to make a visual change (message box, highlight text field, beep, something like that) for the user to see.

Doing something arbitrary when the user's intent is ambiguous is almost always a bad idea. Doing so without logging the fact is perverse. The log doesn't need to be shown to the user; in fact, you don't need to be able to persist it, but you should at least have something in memory you can look at when you are debugging and wondering what the heck just happened.

\$\endgroup\$
  • \$\begingroup\$ Quite a good answer, I like it, but since this is a bounty, I expect more to clean up my class. (Like, I have so many events itself to handle). Also, how can I handle the UI from the second component you are talking about? I mean like, I cannot alter the FXML components outside the ViewModel or the Controller. \$\endgroup\$ – Hassan Althaf Jan 16 '16 at 12:21
1
\$\begingroup\$

changePage()

There won't be the need to keep looping after allowed is set to true. Just break out of the loop.

menuItemClick()

By reverting the condition this.userState.getUser() != null you could return early and save some horizontal spacing which makes it easier to read the code.

inventoryTableViewDetails()

  • Again revert the condition.

  • This Parent viewInventoryDetails = fxmlLoader.load(); is never used. You can remove it.

inventoryTableUpdate()

  • Again revert the condition.

  • This Parent updateInventoryDetails = fxmlLoader.load(); is never used. You can remove it.

inventoryTableDelete()

  • Again revert the condition.

This is also true for the various customerTablexxx() methods.

addOrdersSelectProduct()

Parent selectProduct = fxmlLoader.load(); is never used.

addOrderItem()

  • Again revert the condition

As an example of reverting the condition:

@FXML
private void inventoryTableDelete(MouseEvent event) {
    if (this.productTableView.getSelectionModel().getSelectedItem() != null) {
        int id = this.getSelectedProduct().getId();

        this.productController.deleteProduct(id);
        this.populateProductsTable();
    }
}  

becomes

@FXML
private void inventoryTableDelete(MouseEvent event) {
    if (this.productTableView.getSelectionModel().getSelectedItem() == null) {
        return;
    }

    int id = this.getSelectedProduct().getId();

    this.productController.deleteProduct(id);
    this.populateProductsTable();
}  

Instead of having repeating the code related to the FXMLLoader class you could use a generic method to which you pass the Url like so

private <T> T getController(URL url) {

    FXMLLoader fxmlLoader = new FXMLLoader(url);
    fxmlLoader.load();

    return fxmlLoader.getController();
}  

which then can be called like so

@FXML
private void inventoryTableUpdate(MouseEvent event) throws Exception {

    if (this.productTableView.getSelectionModel().getSelectedItem() == null) {
        return;
    }

    UpdateInventoryDetailsViewModel updateInventoryDetailsViewModel = getController(getClass().getResource("/com/hassanalthaf/telemart/views/UpdateInventoryDetailsView.fxml"));
    updateInventoryDetailsViewModel.show(this.getSelectedProduct(), this);

}
\$\endgroup\$
  • \$\begingroup\$ The details in your answer are too vague. What do you mean by revert \$\endgroup\$ – Hassan Althaf Jan 14 '16 at 12:52
  • \$\begingroup\$ Also, removing the Parent xxx = fxmlLoader.load() gives me a null pointer exception which is why they are there. \$\endgroup\$ – Hassan Althaf Jan 14 '16 at 12:53
  • \$\begingroup\$ I have updated my answer. Regarding the Parent xx if the load() method needs to be called, you should least remove the Parent xxx and just call fxmlLoader.load(); \$\endgroup\$ – Heslacher Jan 14 '16 at 12:55
  • \$\begingroup\$ Oh okay. Will you be updating this answer with more content? \$\endgroup\$ – Hassan Althaf Jan 14 '16 at 12:59
  • \$\begingroup\$ In a short while I will update regarding FXMLLoader \$\endgroup\$ – Heslacher Jan 14 '16 at 12:59

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.