Skip to main content
added 230 characters in body
Source Link

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

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

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, or a Repository targeting domain-specific use cases).

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-DAORepository 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.

  • Data-access logic is polluted with a presentationPresentation concerns

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

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.

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).

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 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.

  • Data-access logic is polluted with a presentation concerns

Please, don't do this, it's very error-prone:

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.

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).

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.

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.

  • Data-access logic is polluted with Presentation concerns

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

Source Link

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());