JavaFX app with User Authentication and SQL Persistence

Question

This JavaFX program is just supposed to allow a user to register a username and password and then have it stored in an SQL database.

There's been some criticism that it's not clean, readable or maintainable, but it still seems to work, so I was looking for criticism from people that are actually in the Software development industry and not just high school teachers.

public class MainController { // region Variables @FXML private Label formText, welcomeText; @FXML private Button login, signup; @FXML private TextField username, email, password, confirmPassword; @FXML private Button forgotPassword, formButton, resetPasswordButton; @FXML private AnchorPane formPage, dashboardPage; // endregion // region Form @FXML private void ChangeForm() { ObservableList<String> shortLogin = login.getStyleClass(), shortSignUp = signup.getStyleClass(); if (shortLogin.contains("active")) { // switching to signup formText.setText("Signup Form"); shortLogin.remove("active"); shortLogin.add("notActive"); shortSignUp.remove("notActive"); shortSignUp.add("active"); confirmPassword.setVisible(true); formButton.setText("Sign Up"); forgotPassword.setVisible(false); } else /*if (shortSignUp.contains("active"))*/ { // switching to login formText.setText("Login Form"); formButton.setText("Login"); shortSignUp.remove("active"); if(!shortSignUp.contains("notActive")) shortSignUp.add("notActive"); shortLogin.remove("notActive"); shortLogin.add("active"); confirmPassword.setVisible(false); formButton.setText("Login"); password.setPromptText("Password:"); forgotPassword.setVisible(true); } ClearForm(); } @FXML private void FormSubmit() { if (ValidForm()) { try { String name = (signup.getStyleClass().contains("active")) ? SQLUtils.Register(username.getText(), password.getText(), email.getText()) : SQLUtils.Login(username.getText(), password.getText(), email.getText()); formPage.setVisible(false); dashboardPage.setVisible(true); welcomeText.setText("Welcome, " + name); ClearForm(); } catch (Exception ignored) { ErrorAlert(Alert.AlertType.ERROR, "SQL Error", "Error Retrieving SQL Information from MainController", "There was an error retrieving the SQL information, or that user doesn't exist."); } } } @FXML private void Forgot() { forgotPassword.setVisible(false); resetPasswordButton.setVisible(true); forgotPassword.setVisible(true); formText.setText("Forgot Password"); formButton.setVisible(false); password.setPromptText("Enter New Password:"); ObservableList<String> shortLogin = login.getStyleClass(); if(shortLogin.contains("active") && !shortLogin.contains("notActive")) { shortLogin.remove("active"); shortLogin.add("notActive"); } } @FXML private void ResetPassword() { if(ValidForm()) { resetPasswordButton.setVisible(false); formButton.setVisible(true); forgotPassword.setVisible(true); formButton.setVisible(true); password.setPromptText("Password:"); ObservableList<String> shortLogin = login.getStyleClass(); formText.setText("Login Form"); shortLogin.remove("notActive"); shortLogin.add("active"); SQLUtils.ResetPassword(username.getText(), password.getText(), email.getText()); ClearForm(); } } // endregion // region Utils private void ClearForm() { username.clear(); email.clear(); password.clear(); confirmPassword.clear(); } private boolean ValidForm() { String emailRegex = "^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9._]+\\.[a-zA-Z]{2,6}$"; String passwordRegex = "^(?=.*[a-z])(?=.*[A-Z])(?=.*[0-9])(?=.*[/~`!@#$%^&*()_+{};:',<.>? =]).{8,}$"; if (username.getText().isEmpty() || email.getText().isEmpty() || password.getText().isEmpty() || (signup.getStyleClass().contains("active") && confirmPassword.getText().isEmpty())) { ErrorAlert(Alert.AlertType.INFORMATION, "Form Validation", "Invalid Fields", "All Fields Must Be Filled In"); return false; } else if (!Pattern.compile(emailRegex).matcher(email.getText()).matches()) { ErrorAlert(Alert.AlertType.INFORMATION, "Form Validation", "Invalid Email", "Please Enter A Valid Email That Contains An '@' And A '.com'"); return false; } else if (!Pattern.compile(passwordRegex).matcher(password.getText()).matches()) { ErrorAlert(Alert.AlertType.INFORMATION, "Form Validation", "Invalid Password", "Please Enter A Valid Password That Contains At Least 8 Characters, 1 Uppercase, 1 Lowercase, 1 Number, and 1 Special Character"); return false; } else if (signup.getStyleClass().contains("active") && !password.getText().equals(confirmPassword.getText())) { ErrorAlert(Alert.AlertType.INFORMATION, "Form Validation", "Passwords Must Match", "Password And Confirm Password Must Match"); return false; } else if (!SQLUtils.ValidInfo(username.getText(), password.getText(), email.getText())) { ErrorAlert(Alert.AlertType.ERROR, "Invalid Info", "That User Does Not Exist", "Please enter valid information for a user that does already exist."); return false; } return true; } public static void ErrorAlert(Alert.AlertType type, String title, String headerText, String contentText) { Alert alert = new Alert(type); alert.setTitle(title); alert.setHeaderText(headerText); alert.setContentText(contentText); alert.showAndWait(); } @FXML private void LogOut() { formPage.setVisible(true); dashboardPage.setVisible(false); welcomeText.setText("Welcome, NAME HERE"); } // endregion // region Window Settings @FXML private void Minimize(ActionEvent event) { ((Stage) ((Button) event.getSource()).getScene().getWindow()).setIconified(true); } @FXML private void Close() { System.exit(0); } // endregion } public class SQLUtils { // region Main Methods public static String Login(String username, String password, String email) { String sql = "select * from users_table where username = ? and password = ? and email = ?"; RunSQL(sql, username, password, email, true); return username; } public static String Register(String username, String password, String email) { String sql = "insert into users_table (username, password, email) values (?, ?, ?)"; RunSQL(sql, username, password, email, false); return username; } public static void ResetPassword(String username, String newPassword, String email) { String sql = "update users_table set password=? where username=? and email=?;"; RunSQL(sql, newPassword, username, email, false); } // endregion // region Utils private static Connection ConnectDB() { try { return DriverManager.getConnection("jdbc:mysql://localhost:3306/login_and_register", "root", "password"); } catch (Exception ignored) { MainController.ErrorAlert(Alert.AlertType.ERROR, "SQL Error", "Error Retrieving SQL Information", "Information could not be retrieved"); } return null; } public static boolean ValidInfo(String username, String password, String email) { String sql = "select * from users_table where username = ? and password = ? and email = ?"; Connection connect = ConnectDB(); if (connect == null) return false; try (PreparedStatement prepared = connect.prepareStatement(sql)) { prepared.setString(1, username); prepared.setString(2, password); prepared.setString(3, email); prepared.executeQuery(); System.out.println("working"); // FORM ALWAYS RESULTS IN WORKING, EVEN WHEN USER IS INVALID, DOES NOT ADD TO TABLE THO return true; } catch (Exception ignored) { MainController.ErrorAlert(Alert.AlertType.ERROR, "Error", "Error Running SQL", "There was an error running the SQL information, or that user doesn't exist."); } System.out.println("not working"); return false; } private static void RunSQL(String sql, String username, String password, String email, boolean query) { Connection connect = ConnectDB(); if (connect == null) return; try (PreparedStatement prepared = connect.prepareStatement(sql)) { prepared.setString(1, username); prepared.setString(2, password); prepared.setString(3, email); if (query) prepared.executeQuery(); else prepared.executeUpdate(); } catch (SQLException ignored) { MainController.ErrorAlert(Alert.AlertType.ERROR, "SQL Error", "Error Retrieving SQL Information, from RUNSQL", "There was an error retrieving the SQL information."); } catch (Exception ignored) { MainController.ErrorAlert(Alert.AlertType.ERROR, "Error", "Error Running SQL", "There was an error running the SQL information, or that user doesn't exist."); } } // endregion } 

Answer 1

superfluous comments

 // region Main Methods ... // endregion [Main Methods] // region Utils ... // endregion [private utility methods] 

Please delete these comments. They are unhelpful, and I'm concerned that a future maintainer will change a {public, private} scope without making a region adjustment. Let the language speak for itself. The comments are redundant with scoping keywords.

hash

This is insanity:

 String sql = "select * from users_table where username = ? and password = ? and email = ?"; 

There is a whole literature on this topic; here is one summary. It says, "store an argon2id hash, rather than the cleartext password the user typed in."

Please do not design or operate security code which stores cleartext passwords. And definitely do not encourage students to do so.

If we are trying to "simplify for didactic purposes", then either call out to some opaque helper library which is out-of-scope for the lesson, or remove the notion of "password" from the lesson.

nit: I would normally expect the username to be a PK. It seems adventurous to me and to future maintainers that you let (alice, [email protected]) login with "password123" and you also let (alice, [email protected]) login with "password456". Write an explanatory comment, or remove that third parameter from the signature.

misleading case

This is harder to read than it ought to be:

 RunSQL(sql, username, password, email, true); 

There are standard java naming conventions which explain that

Methods should be verbs, ... with the first letter lowercase,

Please pick another name, perhaps query(). Similar remarks for login(), register(), etc.

Within the helper we see a query boolean where isQuery was intended.

nit: the ...from RUNSQL diagnostic is inaccurate.

poor Design of Public API

A private helper unconditionally makes three prepared.setString() calls, which apparently drove the design decision to have login() require both username and email. Don't let internal implementation details dictate the shape of the Public API. Design that first, and let helpers discover downstream requirements as needed, such as supplying a variable number of parameters in a query.

poor use of exceptions

Within the validInfo() helper, the contract that prepared.executeQuery() follows is it shall return zero or more rows. Or it doesn't complete at all, for example if someone did a DROP on the users_table.

Your helper does too little -- it neglects to count whether a single result row came back.

 } catch (Exception ignored) { MainController.ErrorAlert(Alert.AlertType.ERROR, "Error", ... 

Up at the call site we see this:

 } else if (!SQLUtils.ValidInfo(username.getText(), password.getText(), email.getText())) { ErrorAlert(Alert.AlertType.ERROR, "Invalid Info", ... 

The helper is trying to catch too much.

Things could have gone badly in more than one way, and the validForm() caller will notify the user if needed. Let the various exceptions bubble up the call stack, so validForm() can deal with them as it sees fit. Low level routines are typically not the right place to "recover" from an error. So-called "checked exceptions" tend to make it difficult to learn this concept if java is the first place you've encountered exceptions.

inappropriate trinary operator

In formSubmit() please change this one-liner:

 String name = (signup.getStyleClass().contains("active")) ? SQLUtils.Register(username.getText(), password.getText(), email.getText()) : SQLUtils.Login(username.getText(), password.getText(), email.getText()); 

It's absurdly unreadable. Take advantage of the else keyword when you split across multiple lines.

inappropriate regexes

 String emailRegex = "^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9._]+\\.[a-zA-Z]{2,6}$"; String passwordRegex = "^(?=.*[a-z])(?=.*[A-Z])(?=.*[0-9])(?=.*[/~`!@#$%^&*()_+{};:',<.>? =]).{8,}$"; 

Email addresses look like: localpart@globalpart

I haven't seen your Requirements document, so I do not yet understand why you prohibit unicode characters within localpart. There are lots of human names, and hence lots of email addresses, which feature accented vowels and scripts such as Hindi, Arabic, Chinese. If you choose to support only a subset of modern email technology, then use an identifier like restrictedEmail or limitedEmail, and document the restrictions. In general you need to ask a recipient email server whether a given localpart is "good".

The globalpart expression is just silly. There are more TLDs than just {com, net, org}. For reasons I do not yet understand you prohibit .academy, .accountant, .apartments, .associates, and, well, I could go on. RFC 7085 describes a bunch of dotless domain which support email.

Rather than a regex, prefer to issue three DNS queries for globalpart. Ask whether the domain has an MX, A, or AAAA record. This can be done quite quickly, and there's no need to contact port 25 for this check. It can help a user determine they made a typographic error.

The only real way to validate a user's address is to email them a GUID link and see if they click on your link, demonstrating knowledge of the GUID.

If your true goal in this project is to detect common errors such as pasting wrong thing into the wrong box, then a regex as simple as .+@.+ could help with much of that.

state machine

In changeForm() there are some constraints you are trying to enforce, such as "exactly one pane shall be visible", something like that. Make it explicit, by writing down /** one or more sentences */. Consider drawing a statechart diagram if you feel that would be an aid to future maintainers.

Answer 2

Connection Management

None of the methods in the SQLUtils which creates a new Connection takes care about closing it, which is a serious issue. Instead of accumulating idle connections on the server-side, you should release database connections as soon as possible.

Just closing a Statement is not enough, it will not cause the underlying Connection to be closed.

^{Closing a Statement will close only the ResultSet associated with it (unless you encounter a quirky behavior of a JDBC driver that doesn't comply with the linked spec, which sometimes happens).}

So, each time you acquire a Connection instance, it should be declared as a resource in the try-statement of a try-with-resources, to ensure that it will be closed.

Design Issues

The major issue is that there's no clear Separation of Concerns between your components.

• Underdeveloped abstractions / Missing abstractions

You need proper abstractions responsible for communicating with the database (not a bag of static methods). To improve your design, instead of SQLUtils you can implement a Data access object with basic CRUD functionality, or a Repository targeting domain-specific use cases.

And you're clearly missing a service, login() is not what abstractions in the data layer are responsible for. A separate abstraction (an application service) should be responsible for user-management.

^{Note that each domain type should have its own dedicated persistence-related abstraction, whether it is a Repository or a plain DAO.}

And this service will depend on the user-Repository and a component responsible for password-hashing.

J_H has already pointed out that you should not store password in plain text, the component I'm was talking about will encapsulate a cryptographic function (such as BCrypt or Argon2) and expose methods for generating hash and verifying that a given password matches the hash retrieved from the database. You can take an inspiration from the PasswordEncoder from Spring Security project.

• Methods lacking clear intent

Methods login() and validInfo() are very odd beasts in the data layer, their names look vied here. And queries they perform don't match business use cases.

Both of them should be replaced with something along the lines findUserByUsername() (if usernames are unique, or by email, depending on the invariants of your application), that expects a single parameter and returns an instance of User constructed based on the fetched data (the rest should not be a responsibility of this class).

If you implement thoroughly each use case, it'll become apparent that method runSQL() is not useful anymore and by its nature it's absolutely arbitrary. If you want to escape from the verbosity of raw JDBC, consider introducing JDBC or JPA framework.

• Data-access logic is polluted with Presentation concerns

Classes talking to a database should not know anything about UI. Nothing like this should appear in your data lair:

MainController.ErrorAlert("display some message to the user"); 

To avoid this coupling, you can an exception which will be processed by a global exception handler. Frameworks like Spring would simplify this task, but you can also do it using method Thread.setDefaultUncaughtExceptionHandler():

Thread .currentThread() .setUncaughtExceptionHandler(<ClassName>::handleUncaught) 

private static void handleUncaught(Thread thread, Throwable throwable) { // error-handling logic here; // use Java 21 Pattern Matching for switch expressions // to determine what action to perform based on the exception type } } 

Code Style

Please, don't do this, it's very error-prone. Use { } in such cases:

... formButton.setText("Login"); shortSignUp.remove("active"); if(!shortSignUp.contains("notActive")) shortSignUp.add("notActive"); shortLogin.remove("notActive"); shortLogin.add("active"); ... 

And by convention, there always should a blankspace between a keyword (such as if) and parentheses.

Don't force code readers scrolling to the right:

String name = (signup.getStyleClass().contains("active")) ? SQLUtils.Register(username.getText(), password.getText(), email.getText()) : SQLUtils.Login(username.getText(), password.getText(), email.getText());