3
\$\begingroup\$

I made an CRUD application in Java with JavaFX, H2 Embedded Database and Maven. Can you review my code quickly? Is it well written? What should I improve on it? I don't know if my code is written correctly, I'm a beginner in Java programming and I'm looking for advices. Where should I look for reviews of my code?

For more information, see the github repository

Here is my code:

public class Main extends Application {
    @Override
    public void start(Stage stage) {
        try {
            FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("/view/MainView.fxml"));
            Scene scene = new Scene(fxmlLoader.load(), 1080, 720);
            scene.getStylesheets().add(Objects.requireNonNull(getClass().getResource("/css/style.css")).toExternalForm());
            Image icon = new Image(Objects.requireNonNull(getClass().getResourceAsStream("/images/icon.png")));
            stage.getIcons().add(icon);
            stage.setTitle("Finance Manager");
            stage.setResizable(false);
            stage.setScene(scene);
            stage.show();
        }
        catch(Exception e) {
            e.printStackTrace();
        }
    }
    public static void main(String[] args) {
        launch();
    }
}
import lombok.AllArgsConstructor;
import lombok.Getter;

@Getter
@AllArgsConstructor

public class Data {
    private final int id;
    private final String name;
    private final String category;
    private final float price;
    private final String date;
}
public class MainController implements Initializable {

    @FXML
    private TextField total;
    @FXML
    private TextField search;
    @FXML
    private TextField errorField;

    @FXML
    private TextField nameField;
    @FXML
    private MenuButton categoryField;
    @FXML
    private TextField priceField;
    @FXML
    private DatePicker dateField;

    @FXML
    private TableView<Data> table;
    @FXML
    private TableColumn<Data, String> nameColumn;
    @FXML
    private TableColumn<Data, String> categoryColumn;
    @FXML
    private TableColumn<Data, Float> priceColumn;
    @FXML
    private TableColumn<Data, Date> dateColumn;

    @Override
    public void initialize(URL url, ResourceBundle resourceBundle) {
        showData();
        dateField.setValue(LocalDate.now());
    }
    @FXML
    private void addBtn() {
        try {
            if(categoryField.getText().equals("Select a category")) {
                throw new NumberFormatException();
            }
            String query =
                    "INSERT INTO DATA VALUES (default,'" + nameField.getText() + "','" + categoryField.getText() +
                            "'," + Float.parseFloat(priceField.getText()) + ",'" + dateField.getValue().toString() +
                            "')";
            executeUpdate(query);
            showData();
            errorField.clear();
        }
        catch(NumberFormatException e) {
            errorField.setText("INCORRECT DATA");
        }
    }
    @FXML
    private void deleteBtn() {
        Data data = table.getSelectionModel().getSelectedItem();
        int id = data.getId();
        String query =
                "DELETE FROM DATA WHERE ID="+id+";";
        executeUpdate(query);
        showData();
    }
    @FXML
    private void updateBtn() {
        try {
            if(categoryField.getText().equals("Select a category") || priceField.getText().startsWith("0")) {
                throw new NumberFormatException();
            }
            Data data = table.getSelectionModel().getSelectedItem();
            int id = data.getId();
            String query =
                    "UPDATE  DATA SET NAME ='" + nameField.getText() + "', CATEGORY = '" + categoryField.getText() + "'," + " PRICE = " +
                            Float.parseFloat(priceField.getText()) + ", DATE = '" + dateField.getValue().toString() + "' " +
                            "WHERE id = " + id + "";
            executeUpdate(query);
            showData();
            errorField.clear();
        }
        catch(NumberFormatException e) {
            errorField.setText("INCORRECT DATA");
        }
    }
    @FXML
    private void onMouseClickedTable() {
        try {
            Data data = table.getSelectionModel().getSelectedItem();
            nameField.setText(data.getName());
            categoryField.setText(data.getCategory());
            priceField.setText(String.valueOf(data.getPrice()));
            dateField.setValue(LocalDate.parse(data.getDate()));
        }
        catch (NullPointerException ignored) {
        }
    }
    @FXML
    private void category(ActionEvent event) {
        String category = ((MenuItem) event.getSource()).getText();
        switch (category) {
            case "food" -> categoryField.setText("food");
            case "cosmetics" -> categoryField.setText("cosmetics");
            case "medicines" -> categoryField.setText("medicines");
            case "bills" -> categoryField.setText("bills");
            case "other" -> categoryField.setText("other");
        }
    }
    private ObservableList<Data> getDataList() {
        ObservableList<Data> dataList = FXCollections.observableArrayList();

        DBConnection DBConnection = new DBConnection();
        Connection connection = DBConnection.getConnection();
        try {
            PreparedStatement statement = connection.prepareStatement("SELECT * FROM DATA");
            ResultSet result = statement.executeQuery();

            while (result.next()) {
                dataList.add(new Data(
                        result.getInt("id"),
                        result.getString("name"),
                        result.getString("category"),
                        result.getFloat("price"),
                        result.getString("date"))
                );
            }
            connection.close();
        } catch (SQLException e) {
            e.printStackTrace();
        }
        return dataList;
    }
    private void showData() {
        nameColumn.setCellValueFactory(new PropertyValueFactory<>("name"));
        categoryColumn.setCellValueFactory(new PropertyValueFactory<>("category"));
        priceColumn.setCellValueFactory(new PropertyValueFactory<>("price"));
        priceColumn.setCellFactory(price -> new TableCell<>() {
            @Override
            protected void updateItem(Float aFloat, boolean b) {
                super.updateItem(aFloat, b);
                if (aFloat == null || b) {
                    setText(null);
                } else {
                    setText(String.format("%.2f", aFloat));
                }
            }
        });

        dateColumn.setCellValueFactory(new PropertyValueFactory<>("date"));

        searchData();
        dateColumn.setSortType(TableColumn.SortType.DESCENDING);
        table.getSortOrder().add(dateColumn);
        table.sort();
    }
    private void executeUpdate(String query) {
        try {
            DBConnection DBConnection = new DBConnection();
            Connection connection = DBConnection.getConnection();

            PreparedStatement statement = connection.prepareStatement(query);
            statement.executeUpdate();

            connection.close();
        } catch (SQLException e) {
            e.printStackTrace();
        }
    }
    private void searchData() {
        FilteredList<Data> filteredData = new FilteredList<>(getDataList(), b -> true);
        search.textProperty().addListener((observableValue, oldValue, newValue) -> filteredData.setPredicate(Data -> {
            if(newValue.isEmpty() || newValue.isBlank()) {
                return true;
            }
            String lowerCase = newValue.toLowerCase();

            if(Data.getName().toLowerCase().contains(lowerCase)) {
                return true;
            }
            else if(Data.getCategory().toLowerCase().contains(lowerCase)) {
                return true;
            }
            else if(String.valueOf(Data.getPrice()).toLowerCase().contains(lowerCase)) {
                return true;
            }
            else return String.valueOf(Data.getDate()).toLowerCase().contains(lowerCase);
        }));
        SortedList<Data> sortedData = new SortedList<>(filteredData);
        sortedData.comparatorProperty().bind(table.comparatorProperty());
        table.setItems(sortedData);

        sortedData.addListener((ListChangeListener.Change<? extends Data> c) -> setTotal());
        setTotal();
    }
    private void setTotal() {
        float totalAmount = 0;
        totalAmount = table.getItems().stream().map(
                Data::getPrice).reduce(totalAmount, Float::sum);
        total.setText("Total: "+String.format("%.2f", totalAmount));
    }
}
public class DBConnection {
    String url = "jdbc:h2:./src/main/resources/db/data";
    String username = "sa";
    String password = "123";

    public Connection getConnection() {
        try {
            return DriverManager.getConnection(url, username, password);
        }
        catch (SQLException e) {
            e.printStackTrace();
        }
        return null;
    }
}
\$\endgroup\$
6
  • \$\begingroup\$ Looks nice. Do you host this project, say, on GitHub where reviewers can access the icon PNG among other resources? \$\endgroup\$
    – coderodde
    Commented Aug 29, 2023 at 15:31
  • \$\begingroup\$ Also, it would benefit reviewers to have a bit more information about the code in the description. From the help center page How to ask: "You will get more insightful reviews if you not only provide your code, but also give an explanation of what it does. The more detail, the better." \$\endgroup\$ Commented Aug 29, 2023 at 15:38
  • 1
    \$\begingroup\$ Yes, you can find it here: github.com/damianlebiedz/Finance-Manager-CRUD- \$\endgroup\$
    – Damian
    Commented Aug 29, 2023 at 15:48
  • \$\begingroup\$ Is there anything to improve here? \$\endgroup\$
    – Damian
    Commented Aug 29, 2023 at 15:50
  • \$\begingroup\$ I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate. \$\endgroup\$ Commented Aug 29, 2023 at 19:24

1 Answer 1

2
\$\begingroup\$

accurate to the penny

public class Data {
    ...
    private final float price;

Consider representing that as a BigDecimal scaled integer, to avoid FP rounding issues. Representing a dollar-twenty as 1.1999999999999999555910790149937383830547332763671875 seems a regrettable choice. Suppose someone's account has thirty cents, and we spend twenty cents and then a dime: .3 - .2 - .1 The account balance has gone negative! We can do better.

Postgres and many relational databases offer scaled integer support via the NUMERIC sql keyword. H2's DECIMAL documentation specifically calls out:

This data type is recommended for storing currency values.

and explains that what you chose "should not be used to represent currency values, because of rounding problems."

Alternatively you might elect to represent price as an Integer number of pennies.

    private final String date;

Another odd choice, but I imagine you have your reasons for preferring VARCHAR over DATE. As long as you stick with an ISO 8601 representation the lexical ordering will work out OK.


manifest constants

    public void start(Stage stage) {
            ...
            Scene scene = new Scene(fxmlLoader.load(), 1080, 720);

Please give names to those magic numbers, perhaps WIDTH and HEIGHT.


production SQL

            String query =
                    "INSERT INTO DATA VALUES (default,'" + nameField.getText() + "','" + categoryField.getText() +
                            "'," + Float.parseFloat(priceField.getText()) + ",'" + dateField.getValue().toString() +
                            "')";

I would have called it String insert since it's not a SELECT query, but fine, we'll let that detail slide.

This INSERT is fine for interactive data exploration. But please name the columns when you put it into production.

Why? Sooner or later the table schema will change. Columns get re-ordered, or new ones are added. We want that to trigger an error here rather than silently defaulting. And we want Author's Intent to be crystal clear to that poor maintenance engineer who will revisit this statement a few months down the road.


swallowing exceptions

    public void start(Stage stage) {
        ...
        catch(Exception e) {
            e.printStackTrace();
        }

Not the worst bit of java code that I've ever seen. But it doesn't seem especially helpful. Consider re-throwing: throw new RuntimeException(e);

    private ObservableList<Data> getDataList() {
        ObservableList<Data> dataList = FXCollections.observableArrayList();
        ...
        } catch (SQLException e) {
            e.printStackTrace();
        }
        return dataList;

The callers, showData() and searchData(), have no idea that the zero-length return value corresponds to an error rather than truth stored in a table. There is no way for a caller to correctly fulfill its contract.

Rather than swallowing an exception, this code really ought to be letting it propagate up the call stack. Either someone will know how to deal with it, or we (correctly) will display a stack trace.

Similar craziness happens in executeUpdate().

In getDataList() we potentially assign Connection connection = null and then NPE on connection.prepareStatement(), mixing that NPE in with the various other kinds of errors a SELECT might generate.


nouns for names

            else if(Data.getCategory().toLowerCase().contains(lowerCase)) {

Here we're essentially comparing against lowerCaseNewValue or perhaps lcNewValue. The adjective identifier turned out to not be very convenient. Consider putting the long name up front:

        ... .addListener((..., ..., mixedNewValue) -> ... -> { ...
            String newValue = mixedNewValue.toLowerCase();

Downcasing price seems a curious operation, unless you really want to keep it as FP and are concerned about uppercase E appearing in scientific notation.

Downcasing date suggests that you're not using an ISO 8601 representation. You might want to revisit that decision.

Also, please write one sentence of /** javadoc */ for searchData(), explaining what it does. I'm sure there's a good reason for " " (two blanks) acting as a match-all wildcard; I just haven't quite seen it, yet.


automated testing

I see no unit tests at https://github.com/damianlebiedz/Finance-Manager-CRUD-/tree/main/src . Spotting regressions as they're committed will prove valuable to you as this small project evolves into a larger one.


This codebase appears to achieve many of its design goals.

I would be willing to delegate or accept maintenance tasks on it.

\$\endgroup\$

Your Answer

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

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