Login module with JDBC
Checking return values

A mistake I see in multiple places is not checking the return values of operations.

Take for example the part of getting the user ID:


PreparedStatement prepStmt = conn.prepareStatement("SELECT userId FROM SiteUser WHERE name =?");
prepStmt.setString(1, username);
ResultSet rs = prepStmt.executeQuery();
rs.next();
return rs.getInt(1);



Before getting values from a ResultSet instance,
you should always check the return value of rs.next(),
as explained in the official JDBC tutorial.

Without that check, the rs.getInt(1) statement will raise SQLException when there is no matching user.
Since you wrapped the block of code in try { ... } catch (SQLException e) { ... }, the exception will be handled, but this is not appropriate use of exceptions.

The purpose of the try-catch is not to handle the case of no matching user.
Its purpose is to handle unexpected errors during the database operation.
If you rely on this try-catch to handle the case of no matching user,
that's effectively using exceptions for flow control.
That's not the intended use, and it's also inefficient,
as it's slower than using a conditional to check the value of rs.next().

I suggest to review the entire code, look for statements where a function returns a value but you don't use it. Some IDEs for example IntelliJ will give you a warning on statements like that.



Another form of not checking the return value is in the process of registering a user. Consider this snippet from the register method:


insertNewUser(username, pass);
createAccountForUser(username);



The insertNewUser doesn't return anything,
but probably it should.
Inserting a new user might not succeed,
and it would be incorrect to create an account if the user could not be created.

Potentially unsafe register

The implementation looks prone to a race condition:
in between the moment of checking if the username exists,
and the moment of inserting the user,
another process or program may have inserted a user with the same name.
In fact,
unless you wrap these two operations within a transaction,
there's no way to safely guarantee that the insert operation will succeed.

I'm not sure if you have a unique index on the username column.
If you do, then you could skip checking if the user exists,
attempt to insert.
If the insert is successful, the user did not exist, it was successfully inserted, and you can continue creating the account.
If the insert fails, whether because the user already exists, or due to other database errors, you can abort further processing.

Connection management

Creating connections is an expensive operation.
Avoid creating new connections unnecessarily as much as possible.
It might be a good idea to manage the connection outside this class.
Or if you really want to create it in this class,
make sure that a chain of operations reuses the same connection,
rather than recreating it in each step.

Ordering of operations

The register method first checks if the specified username exists,
and then validates the username and the password against some built-in rules.
It would be better to perform these operations in the opposite order.
The validation logic is a cheap operation,
database lookups are expensive.

Confusing member and local variables

Some methods reuse the conn field,
others create it as a local variable.
This is confusing.