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're dialing with a quirky JDBC driver that doesn't comply with the spec).
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 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.
And this service will depend on the user-DAO and a component responsible for password-hashing.
J_H has already pointed out that you should not store password in a 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 a 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:
...
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());