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);
}
1 Answer 1
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
andprivate 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 inAddingScreenController#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 PreparedStatement
s. 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.