4
\$\begingroup\$

This is my second project. I've wanted to create a little client for my local database, so I can get familiar with databases. I've created a client for one db table using JavaFX. I want to know what I need to change especially in terms of designing methods and classes, since I feel like it is pretty horrible, but I just don't know how exactly I should split everything.

Login Controller Class

public class LoginController 
 private databaseConnection conn = new databaseConnection("student","test");
 private PreparedStatement mySt = null;
 private Statement st = null;
 private boolean noConnection;
 @FXML
 private Button button;
 @FXML
 private TextField username;
 @FXML
 private TextField password;
 @FXML
 private Label invalidPass;
 @FXML
 private void buttonAction (ActionEvent event) throws IOException, SQLException{
 Parent mainPageParent = FXMLLoader.load(getClass().getResource("fxml/DatabaseUi.fxml"));
 Scene mainPageScene = new Scene(mainPageParent);
 Stage loginStage = (Stage) ((Node) event.getSource()).getScene().getWindow();
 if(this.isValidLogin()){
 loginStage.hide();
 loginStage.setScene(mainPageScene);
 loginStage.show();
 }else{
 username.clear();
 password.clear();
 if(noConnection){
 invalidPass.setText("No connection to the Database");
 }else{
 invalidPass.setText("Invalid username or password !");
 }
 }
 }
 private boolean isValidLogin() throws IOException, SQLException {
 try {
 conn.getConnection();
 mySt = conn.getConnection().prepareStatement("SELECT * FROM users WHERE username=? AND password=?");
 mySt.setString(1, username.getText());
 mySt.setString(2, password.getText());
 ResultSet result = mySt.executeQuery();
 if(result.next()){
 return true;
 }else{
 return false;
 }
 } catch (SQLException e ) {
 this.noConnection=true;
 e.printStackTrace();
 }
 return false;
 }
}

Main Database Ui class

public class DatabaseUiController {
 private databaseConnection conn = new databaseConnection("student","test");
 private PreparedStatement mySt = null;
 private Statement st = null;
 private ResultSet result = null;
 private ObservableList<Customer> customers=FXCollections.observableArrayList();
 private Connection myConn =null;
 @FXML
 private Button searchButton;
 @FXML
 private Button addButton;
 @FXML
 private Button updateButton;
 @FXML
 private Button deleteButton;
 @FXML
 private TextField searchField;
 @FXML
 private TableView<Customer> databaseTable;
 @FXML
 private TableColumn columnId;
 @FXML
 private TableColumn columnFirstName;
 @FXML
 private TableColumn columnLastName;
 @FXML
 private TableColumn columnEmail;
 @FXML 
 private Label warningLabel;
 @FXML
 private void searchInformation(ActionEvent event) {
 this.tableRefresh();
 }
 @FXML
 private void addButtonAction(ActionEvent event){
 try {
 Parent parentMainAddingScreen = FXMLLoader.load(getClass().getResource("fxml/AddingScreen.fxml"));
 Scene mainAddingScreenScene = new Scene(parentMainAddingScreen);
 Stage addingStage = new Stage();
 addingStage.setTitle("Add customer");
 addingStage.setScene(mainAddingScreenScene);
 addingStage.showAndWait();
 this.tableRefresh();
 } catch (IOException e) {
 e.printStackTrace();
 }
 }
 @FXML
 private void updateButtonAction(ActionEvent event){
 try {
 FXMLLoader loader = new FXMLLoader(getClass().getResource("fxml/UpdateScreen.fxml"));
 Stage updateStage = new Stage();
 updateStage.setScene(
 new Scene((Pane) loader.load()));
 UpdateScreenController controller = loader.getController();
 Customer selectedCustomer = databaseTable.getSelectionModel().getSelectedItem();
 controller.setSelectedCustomer(selectedCustomer);
 if(selectedCustomer==null){
 warningLabel.setText("You must select the Customer!");
 }else{
 controller.fillTextFields();
 updateStage.showAndWait();
 this.tableRefresh();
 }
 } catch (IOException e) {
 e.printStackTrace();
 }
 }
 @FXML
 private void deleteEvent(ActionEvent event){
 try {
 Connection myConn = conn.getConnection();
 Customer selectedItem = databaseTable.getSelectionModel().getSelectedItem();
 mySt = myConn.prepareStatement("DELETE FROM `customer`.`customer` WHERE id = ?;");
 if(selectedItem !=null){
 if(selectedItem.getFirstName() != null) {
 mySt.setLong(1, selectedItem.getId());
 mySt.executeUpdate();
 this.tableRefresh();
 }
 }
 } catch (SQLException e) {
 // TODO Auto-generated catch block
 e.printStackTrace();
 }finally{
 try { if (result != null) result.close(); } catch (Exception e) {};
 try { if (st != null) st.close(); } catch (Exception e) {};
 //try { if (conn != null) myConn.close(); } catch (Exception e) {};
 }
 }
 public void tableRefresh() {
 customers.clear();
 try {
 Connection myConn = conn.getConnection();
 mySt = myConn.prepareStatement("SELECT * FROM Customer");
 result = mySt.executeQuery();
 if(!searchField.getText().isEmpty() ){
 while(result.next()){
 if( result.getString(2).toLowerCase().contains((searchField.getText().toLowerCase())) || 
 result.getString(3).toLowerCase().contains((searchField.getText().toLowerCase() ))){
 customers.add(new Customer(result.getInt(1), result.getString(2),
 result.getString(3), result.getString(4)));
 }
 }
 }else{
 while(result.next()){
 customers.add(new Customer(result.getInt(1), result.getString(2),
 result.getString(3), result.getString(4)));
 }
 }
 if(columnId.getCellValueFactory() ==null){
 columnId.setCellValueFactory(new PropertyValueFactory<>("id"));
 columnFirstName.setCellValueFactory(new PropertyValueFactory<>("firstName"));
 columnLastName.setCellValueFactory(new PropertyValueFactory<>("lastName"));
 columnEmail.setCellValueFactory(new PropertyValueFactory<>("email"));
 databaseTable.setItems(customers);
 }
 } catch (SQLException e) {
 warningLabel.setText("Lost connection to the database");
 e.printStackTrace();
 } finally{
 try { if (result != null) result.close(); } catch (Exception e) {};
 try { if (st != null) st.close(); } catch (Exception e) {};
 try { if (conn != null) myConn.close(); } catch (Exception e) {};
 }
 }
}

Adding Screen Controller Class

public class AddingScreenController {
private databaseConnection conn = new databaseConnection("student","test");
private PreparedStatement stmt =null;
private Connection myConn=null;
@FXML
private TextField firstNameTextField;
@FXML
private TextField lastNameTextField;
@FXML
private TextField emailTextField;
@FXML
private Button okButton;
@FXML
private Button cancelButton;
@FXML
private Label warningLabel;
@FXML
private void okbuttonAction (ActionEvent event){
 if(firstNameTextField.getLength() <1 || lastNameTextField.getLength() <1 || emailTextField.getLength()<1){
 warningLabel.setText("Every field must be filled !");
 }else{
 try {
 myConn= conn.getConnection(); 
 stmt = myConn.prepareStatement("INSERT INTO `customer`.`customer` (`firstName`, `lastName`, `email`) "
 + "VALUES (?,?,?)");
 stmt.setString(1, firstNameTextField.getText()); 
 stmt.setString(2, lastNameTextField.getText()); 
 stmt.setString(3, emailTextField.getText()); 
 stmt.executeUpdate();
 this.hideAddingScreen(event);
 } catch (SQLException e) {
 // TODO Auto-generated catch block
 e.printStackTrace();
 } finally{
 // try { if (result != null) result.close(); } catch (Exception e) {};
 try { if (stmt != null) stmt.close(); } catch (Exception e) {};
 try { if (myConn != null) myConn.close(); } catch (Exception e) {};
 }
 }
}
@FXML
private void cancelbuttonAction (ActionEvent event) throws IOException{
 this.hideAddingScreen(event);
}
private void hideAddingScreen (ActionEvent event){
 Stage addStage = (Stage) ((Node) event.getTarget()).getScene().getWindow();
 addStage.hide();
}

Update Controller Class

public class UpdateScreenController {
private databaseConnection conn = new databaseConnection("student","test");
private PreparedStatement stmt =null;
private Connection myConn=null;
private Customer selectedCustomer;
@FXML
private TextField firstNameTextField;
@FXML
private TextField lastNameTextField;
@FXML
private TextField emailTextField;
@FXML
private Button okButton;
@FXML
private Button updateButton;
@FXML
private Label warningLabel;
public void setSelectedCustomer(Customer customer){
 this.selectedCustomer=customer;
}
@FXML
private void updateButtonAction(ActionEvent event){
 try {
 if(firstNameTextField.getLength() <1 || lastNameTextField.getLength() <1 || emailTextField.getLength()<1){
 warningLabel.setText("Every field must be filled !");
 } else{
 myConn= conn.getConnection();
 stmt = myConn.prepareStatement("UPDATE customer SET firstName=?, lastName=?, email=? "
 + "WHERE id=?");
 stmt.setString(1, firstNameTextField.getText()); 
 stmt.setString(2, lastNameTextField.getText()); 
 stmt.setString(3, emailTextField.getText()); 
 stmt.setLong(4, selectedCustomer.getId());
 stmt.executeUpdate();
 this.hideUpdateScreen(event);
 }
 } catch (SQLException e) {
 // TODO Auto-generated catch block
 e.printStackTrace();
 } 
}
@FXML
private void cancelButtonAction(ActionEvent event){
 this.hideUpdateScreen(event);
}
private void hideUpdateScreen (ActionEvent event){
 Stage updateStage = (Stage) ((Node) event.getTarget()).getScene().getWindow();
 updateStage.hide();
}
public void fillTextFields(){
 firstNameTextField.setText(selectedCustomer.getFirstName());
 lastNameTextField.setText(selectedCustomer.getLastName());
 emailTextField.setText(selectedCustomer.getEmail());
}

Database Connection Class

public class databaseConnection {
private String username;
private String password;
private Connection conn =null;
private PreparedStatement mySt = null;
private Statement st = null;
private ResultSet result = null;
public databaseConnection(String username,String password){
 this.username=username;
 this.password=password;
}
public Connection getConnection() throws SQLException{
 if(conn ==null){
 conn= DriverManager.getConnection("jdbc:mysql://localhost:3306/customer",username,password);
 }
 return conn;
}

Customer Class

public class Customer {
private int id;
private String lastName;
private String firstName;
private String email;
public Customer(int id, String lastName, String firstName, String email) {
 this.id = id;
 this.lastName = lastName;
 this.firstName = firstName;
 this.email = email;
}
public int getId() {
 return id;
}
public void setId(int id) {
 this.id = id;
}
public String getLastName() {
 return lastName;
}
public void setLastName(String lastName) {
 this.lastName = lastName;
}
public String getFirstName() {
 return firstName;
}
public void setFirstName(String firstName) {
 this.firstName = firstName;
}
public String getEmail() {
 return email;
}
public void setEmail(String email) {
 this.email = email;
}
@Override
public String toString() {
 return String.format("Customer [id=%s, lastName=%s, firstName=%s, email=%s]",
 id, lastName, firstName, email);
}

Main Class

*public class Main extends Application {
@Override
public void start(Stage primaryStage) {
 try {
 Pane root =FXMLLoader.load(getClass().getResource("fxml/Login.fxml"));
 Scene scene = new Scene(root);
 primaryStage.setScene(scene);
 primaryStage.show();
 } catch(Exception e) {
 e.printStackTrace();
 }
}
public static void main(String[] args) {
 launch(args);
}
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Mar 19, 2017 at 23:21
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
  • databaseConnection should be named according to java naming conventions: DatabaseConnection.

  • I dislike strongly that you're not injecting the Connection as dependency and instead new it up in all Controllers. This means that each controller has their own connection instance, which is really bad practice. It results in significantly more connections than necessary, isn't maintainable and you're additionally hardcoding credentials into every single Controller. That's horrible.

  • You're abusing private fields as replacement for variables. Instead of private PreparedStatement mySt = null and private Statement st = null you should be declaring these as variables as close as possible to their usage. This reduces the overhead an instance of your class has in the heap and it makes the code easier to reason about, because there's less things that could be relevant that you need to keep in mind.

  • You only use loginStage in the if-block. Why not look it up only there?

  • isValidLogin can be simplified. It also doesn't close the statement against the database, which can result in "too many open statements". For that we can use the "try-with-resources"-statement. Consider instead the following:

    try (PreparedStatement loginQuery = conn.getConnection().prepareStatement("SELECT ...")) {
     loginQuery.setString(1, username.getText());
     loginQuery.setString(2, password.getText());
     try (ResultSet result = loginQuery.executeQuery()) {
     return result.next();
     }
    } catch (SQLException e) {
     this.noConnection = true;
     e.printStackTrace();
    }
    return false;
    
  • Similar considerations apply to DatabaseUiController. I'm not further reviewing that class though, because apparently formatting it is not necessary. If not even formatting is necessary, it's probably not necessary to even reason about the class ;) Keep in mind that you want to be able to read the code when coming back to it later. It's important to make that easy for yourself and others. That includes formatting it correctly.

    Note that after a cursory glance I'd advise you to read about try-with-resources. That should already bring good simplifications :)

  • The else-Block in AddingScreenController#okbuttonAction needs to be indented correctly. try-with-resources also helps here. Also the variables that got turned into fields should go back to being variables...

  • At this point in UpdateScreenController I feel like a broken record. Same as above again :)

Last words:

The code is pretty clean and correctly uses PreparedStatements. There's some quite outdated java tutorials out there that you seem to have found. Most criticism comes from not using the latest best practice to do things.

There's a sloppy feeling to the code, mostly coming from lacking indentation and capitalization errors in method and class names. Programming is hard work and "Consistency is King".

Good job overall.

answered Dec 4, 2017 at 11:28
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.