Is it good practice to have all my back end in one single file with all functions being static?

Question

package LoginSystem;

import java.sql.Statement;
import java.sql.Timestamp;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.ResultSet;
import java.sql.SQLException;



//data base consists of columns [ID, username, password, numOfPasswordChanges, passwordChangeTimer]
public class DataBaseController {
    private static final int MAX_USERNAME_LENGTH = 20;
    private static final int MIN_USERNAME_LENGTH = 5;

    private static final int MAX_PASSWORD_LENGTH = 30;
    private static final int MIN_PASSWORD_LENGTH = 7;

    public static final int CORRECT_PASSWORD_RETURN_VAL  = 1;
    public static final int INCORRECT_PASSWORD_RETURN_VAL = 2;
    public static final int MAX_PASSWORD_RESETS_REACHED_RETURN_VAL = 3;

    public static final int WEAK_PASSWORD_RETURN_VAL = 4;
    public static final int WEAK_USERNAME_RETURN_VAL = 5;
    public static final int USER_ALREADY_EXISTS_RETURN_VAL = 6;
    public static final int USER_NOT_EXISTS_RETURN_VAL = 7;    
    

    private static final int ID_INDEX = 1;
    private static final int USERNAME_INDEX = 2;
    private static final int PASSWORD_INDEX = 3;
    private static final int PASSWORD_RESET_INDEX = 4;
    private static final int RESET_TIMER_INDEX = 5;

    private static final int MAX_PASSWORD_RESETS = 3;

    private static final long RESET_TIME_TO_ADD = 20000L;
    
    private static final String ROOT_NAME = "root";       
    private static final String DB_PASSWORD = "root";
    private static final String DB_NAME = "mydb";

    public DataBaseController() throws SQLException {

    }

    public static boolean authorizeUser(String username, String password) throws Exception {
        
            Class.forName("com.mysql.jdbc.Driver");
            Connection con = DriverManager.getConnection("jdbc:mysql://localhost:3306/" + DB_NAME, ROOT_NAME,DB_PASSWORD);
            Statement stmt = con.createStatement();  
            String query = "SELECT * FROM users WHERE username = " + "'" + username + "'" + " AND password =  " + "'" + password + "'";
            ResultSet rs = stmt.executeQuery(query);
            boolean userExists = rs.next();
            if(rs.next()) {
                throw new Exception("Duplicate username in database!");
            }
            rs.previous();
            boolean correctLogin = false;
            if(userExists) {
                correctLogin = rs.getString(USERNAME_INDEX).equals(username) && rs.getString(PASSWORD_INDEX).equals(password); 
            }

            return correctLogin;

    }

    public static int register(String username, String password) throws ClassNotFoundException, SQLException {
        if(!checkIfUsernameValid(username)) {
            return WEAK_USERNAME_RETURN_VAL;
        }
        
        if(!checkIfPasswordValid(password)) {
            return WEAK_PASSWORD_RETURN_VAL;
        }
        Class.forName("com.mysql.jdbc.Driver");
        Connection con = DriverManager.getConnection("jdbc:mysql://localhost:3306/" + DB_NAME + "?verifyServerCertificate=false&useSSL=true", ROOT_NAME, DB_PASSWORD);
        Statement stmt = con.createStatement();
        String query = "SELECT * FROM users WHERE username = " + "'" + username + "'";
        ResultSet rs = stmt.executeQuery(query);
        boolean doesUserExists = rs.next();
        
        
        if(!doesUserExists) {
            String numberOfUsersQuery = "SELECT COUNT(*) FROM users";
            
            ResultSet userIDRs = stmt.executeQuery(numberOfUsersQuery);
            String addUserSql = "";

            userIDRs.next();
            int newID = 1 + userIDRs.getInt(1);
            addUserSql = "INSERT INTO users(ID, username, password) VALUES(" + newID +", "  + "'" + username + "'" + ", " + "'" + password + "');";

            stmt.executeUpdate(addUserSql);


        }
        if(doesUserExists) {
            return USER_ALREADY_EXISTS_RETURN_VAL;
        }
        return USER_NOT_EXISTS_RETURN_VAL;
    }

    public static int resetPassword(String username, String oldPassword, String newPassword) throws ClassNotFoundException, SQLException {
        Class.forName("com.mysql.jdbc.Driver");
        Connection con = DriverManager.getConnection("jdbc:mysql://localhost:3306/" + DB_NAME + "?verifyServerCertificate=false&useSSL=true", ROOT_NAME, DB_PASSWORD);
        Statement stmt = con.createStatement();
        String checkUserQuery = "SELECT * FROM users WHERE username = " + "'" + username + "'" + " AND " + "password = " + "'" + oldPassword + "'";
        ResultSet rs = stmt.executeQuery(checkUserQuery);
        if(!rs.next()) {
            return INCORRECT_PASSWORD_RETURN_VAL;
        }

        if(!checkIfPasswordValid(newPassword)) {
            return WEAK_PASSWORD_RETURN_VAL;
        }

        int numOfChanges = rs.getInt(PASSWORD_RESET_INDEX);
        Timestamp usersResetTimer = rs.getTimestamp(RESET_TIMER_INDEX);
        System.out.println(usersResetTimer + " **** " + new Timestamp(System.currentTimeMillis()) + " **** " + usersResetTimer.after(new Timestamp(System.currentTimeMillis())));
        if(!usersResetTimer.after(new Timestamp(System.currentTimeMillis()))) {
            numOfChanges = 0;
        }
        if(numOfChanges >= MAX_PASSWORD_RESETS) {
            return MAX_PASSWORD_RESETS_REACHED_RETURN_VAL;
        }

        
        String query = "UPDATE users SET password = " + "'" + newPassword + "'" + " WHERE username = " + "'" + username + "'"; 
        try {
            stmt.executeUpdate(query);
            numOfChanges += 1;
            query = "UPDATE users SET numOfPasswordChanges = " + numOfChanges + " WHERE username = " + "'" + username + "'";
            stmt.executeUpdate(query);
            Timestamp newUserResetTimer = new Timestamp(System.currentTimeMillis() + RESET_TIME_TO_ADD);
            System.out.println(newUserResetTimer);
            String timeWithoutMillis = newUserResetTimer.toString().split("\\.")[0];
            System.out.println(newUserResetTimer.toString());

            query = "UPDATE users SET passwordChangeTimer = " + "'" + timeWithoutMillis + "'" + " WHERE username = " + "'" + username + "'";
            
            stmt.executeUpdate(query);

            
        } catch (Exception e) {
            e.printStackTrace();
        }
        return CORRECT_PASSWORD_RETURN_VAL;

    }

    private static boolean checkIfUsernameValid(String username) {
        if( username == null || username.length() > MAX_USERNAME_LENGTH || username.length() < MIN_USERNAME_LENGTH)
        {
            return false;
        }

        for(int i = 0; i < username.length(); i++) {
            if(!Character.isLetter(username.charAt(i)) && !Character.isDigit(username.charAt(i))) {
                return false;
            }
        }

        return true;

    }

    private static boolean checkIfPasswordValid(String password) {
        if(password == null || password.length() > MAX_PASSWORD_LENGTH || password.length() < MIN_PASSWORD_LENGTH) {
            return false;
        }

        boolean symbolFlag = false;
        boolean digitFlag = false;
        boolean letterFlag = false;

        for(int i = 0; i < password.length(); i++) {
            if(!Character.isLetter(password.charAt(i)) && !Character.isDigit(password.charAt(i))) {
                symbolFlag = true;
            }

            if(Character.isLetter(password.charAt(i))) {
                letterFlag = true;
            }

            if(Character.isDigit(password.charAt(i))) {
                digitFlag = true;
            }

            if(symbolFlag && digitFlag && letterFlag) {
                return true;
            }
        }

        return false;
    }

    
}

I have a bunch of classes that uses this to register/change password etc... and this is basically all of my backend in one class almost all of it static aside from a simple checks if username and passwords aren't weak (although nothing will change if it were static), so is it good practice? I can add here the rest of the files but all of them are just simple front end swing GUI pages with no server running in this project.

Answer 1

cache your DB connection

    public static boolean authorizeUser(String username, String password) 
            ...
            Connection con = DriverManager.getConnection("jdbc:mysql://localhost:3306/" + DB_NAME, ROOT_NAME, DB_PASSWORD);

It looks like this gets called on pretty much every GET request. MySQL, Oracle, and other backends take a performance hit if you create a brand new connection for each query. Some folks will cache in a locked singleton, some have instances of their application class hang onto it, while others will opt for a connection pool. I will typically use a connection pool in my SqlAlchemy / flask apps. Not knowing your parameters, I won't tell you what to do here. Consider benching how many GETs per second you can get from your app with & without caching connections.

---

report error properly

            String query = "SELECT * FROM users WHERE username = " + "'" + username + "'"
                + " AND password =  " + "'" + password + "'";
            ...
            if (rs.next()) {
                throw new Exception("Duplicate username in database!");
            }

Suppose we attempt authorizeUser("scott", "tiger") which succeeds, and then attempt authorizeUser("scott", "hunter"), for which zero rows match. The userExists = rs.next() positioned us at end-of-results, and the docs say this about the subsequent if (rs.next()) call:

When a call to the next method returns false, the cursor is positioned after the last row. Any invocation of a ResultSet method which requires a current row will result in a SQLException being thrown.

I guess we can call next; next; next repeatedly. But the test should have been if (userExists).

And rather than vanilla raise Exception..., please throw something more specific like IllegalArgumentException, or much better, an app-specific exception that you define. Then a caller that knows how to recover can choose to catch the narrower anticipated error rather than catching broadly.

In any event, the AND password = 'tiger' clause isn't really appropriate. You're delegating the security decision to the RDBMS. Better to just retrieve the user's attributes, such as password, and make the security decision at app level.

---

use prepared statements

"SELECT * FROM users WHERE username = " + "'" + username + "'" ...   

Little Bobby Tables' mom just loves queries like that. Avoid sql injection attacks. Habitually use the bind-parameter interface when submitting queries.

---

salt your credentials

                correctLogin =
                       rs.getString(USERNAME_INDEX).equals(username)
                    && rs.getString(PASSWORD_INDEX).equals(password);

Assume that your users table leaks out at some point to a hacker. Now all those cleartext passwords are exposed, which is Bad, enabling attacks on other servers frequented by your users. We usually store a hash of password to mitigate such a threat. But that still admits of offline dictionary attacks.

Salt your password hashes to make it harder for attackers to launch such an attack.

Answer 2

It depends :)

I will focus on the question itself and not the code i.e. Is it good to cover all in one class.

Good Parts :

• I can see you’ve broken up code into multiple logical functions mostly so it is good.
• It will work so If you're doing quick POC then it is ok.

Bad Parts :

• It violates SRP principle from SOLID I.e. class should have one reason to change.
• Many of your functions also violates SRP like authorizeUser as they get DB connection and also fetches results from the DB.
• It makes code unreadable as lots of things are in one class and class name won’t convey purpose of logic inside it.
• We can’t have common logic and reuse it in different implementations like Getting connection or executing SQL query.
• With static functions it becomes a utility/helper class. It does not follow OOP principles at all like Encapsulation, Abstraction etc.

Answer 3

Following a stand point avoiding unsolicited feedback.

Having everything static it is memory and algorithmic efficient, there is no need of an instance to call the methods or variables, it also sets aside the overhead of taking care about the concern of instance per call that could result in memory overload.

I have to admit I am confused by "good practice" meaning that could be what you need and then it is a "good practice" or according to European Website on Integration it...

... can be defined in multiple ways. However, a thread common to most definitions implies strategies, approaches and/or activities that have been shown through research and evaluation to be effective, efficient, sustainable and/or transferable, and to reliably lead to a desired result.

... and then it is the opposite of "good practice". Differently said whether it is or not it depends on your happiness.

Answer 4

public DataBaseController() throws SQLException {

}

1. once you are creating a static methods, it seems that this is the pattern for this class, so adding private ctor is better way, to force users not to create objects of this class.