3
\$\begingroup\$

I've written this MySQL database class for my Java application and I'd just like some feedback on it before moving onto making my next class.

The idea here is that I have a simple properties file which stores the hostname, username, password, and database name. I then connect to the database (if possible) and return the Connection.

Although I am not really sure what should happen if the properties file doesn't exist, or the MySQL connection fails, should I return Connection which would be null, throw a new exception, or re-throw the existing exception?

package app;

import java.io.IOException;
import java.io.InputStream;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.SQLException;
import java.util.Properties;

public class Database {

    private String hostname;
    private String database;
    private String username;
    private String password;
    private Connection conn;

    public Connection getConnection() {

        if(conn == null) {

            try {

                System.out.println("Getting properties file...");
                Properties prop = new Properties();
                InputStream input = Database.class.getResourceAsStream("database.properties");
                prop.load(input);
                this.hostname = prop.getProperty("hostname");
                this.database = prop.getProperty("database");
                this.username = prop.getProperty("username");
                this.password = prop.getProperty("password");
                input.close();

            } catch (IOException ex) {

                // Error Reading Properties File
                ex.printStackTrace();

            } finally {

                if (this.database != null) {

                    try {
                        System.out.println("Connecting to database...");
                        Class.forName("com.mysql.jdbc.Driver");
                        conn = DriverManager.getConnection("jdbc:mysql://" + this.hostname + "/" + this.database + "?user=" + this.username + "&password=" + this.password);

                    } catch (SQLException ex) {

                        // Error Connecting to Database
                        ex.printStackTrace();

                    } catch (ClassNotFoundException ex) {

                        // Error Connecting to Database
                        ex.printStackTrace();

                    }

                }

            }

        }

        return conn;  

    }

}
\$\endgroup\$
1
\$\begingroup\$

Finally

The "finally" clause is for cleaning up resources and not for continued processing. Java does not prevent your kind of usage but their are some assertions to the finally clause your code does not make use of. So it can be placed in the try-block.

try with resource

Use the try with resource construct when loading the properties from the file. This ensures proper resource closing. In your case "input.close()" will never be called if an exception happens before. So you will introduce a memory leak.

Extract method

You should separate the concerns of loading the properties and connecting to the database.

Exception handling

I suggest to throw all exceptions instead of handling it. Maybe you should wrap it to a custom exception that is not signature relevant (RuntimeException).

The alternative you provided leaves the caller in doubt why he gets null. I guess you currently check for null by the caller when the getConnection()-emtoh gets called. Instead of checking for null you should handle the exception there where some code has to decide what to if no connection could be established.

Avoid null and not useful checks

I suggest to remap missing values to a type specific neutral element (e.g.):

this.hostname = prop.getProperty("hostname", "" /*default value*/);

Furthermore do not check things that other code will check anyway (DriverManager.getConnection). So omit

if (this.database != null)

Encapsulate connection information

Introduce a new structure that represents the information necessary to establish a connection and use it to be independent of "retreiving the data from a properties object" that provides no semantic.

Variable scope

You can reduce the scope of the connection information variables to "method". Once you are connected they will be present within the connection. And you never reinitialize the "conn"-variable.

Extract constants

You have a lot of Strings that should be extracted as constants

String templates

Not neccessarily objective but using template strings will have influence how you express String composition.

Naming

As you are currently supporting a MySQL Database I suggest to rename "Database" to "MySQLDatabase".

Separate concerns of process and output

If you introduce a callback or an observer mechanism you separate the console output from the real doing. You are no longer tied to a console output. You can easily change console output to log file output.

Refactored code

public class MySQLDatabase {


    private static final String DATABASE_PROPERTIES_FILENAME = "database.properties";
    private static final String DRIVER_CLASS_NAME = "com.mysql.jdbc.Driver";
    private static final String MYSQL_CONNECTION_STRING_TEMPLATE = "jdbc:mysql://%1$s/%2$s?user=%3$s&password=%4$s";
    private static final String KEY_PASSWORD = "password";
    private static final String KEY_USERNAME = "username";
    private static final String KEY_DATABASE = "database";
    private static final String KEY_HOSTNAME = "hostname";


    private Connection conn;
    private Callback callback;


    public MySQLDatabase(Callback callback) {
        this.callback = callback;
    }


    private static class ConnectionInfo {

        private String hostname;
        private String database;
        private String username;
        private String password;

        public ConnectionInfo(String hostname, String database, String username, String password) {
            this.hostname = hostname;
            this.database = database;
            this.username = username;
            this.password = password;
        }

        public String getHostname() {
            return hostname;
        }

        public String getDatabase() {
            return database;
        }

        public String getUsername() {
            return username;
        }

        public String getPassword() {
            return password;
        }

    }


    public Connection getConnection() {

        if (conn == null) {

            try {

                ConnectionInfo connectionInfo = getConnectionInfo();

                callback.onConnectingToDatabase();

                Class.forName(DRIVER_CLASS_NAME);

                String mySQLConnectionString = String.format(MYSQL_CONNECTION_STRING_TEMPLATE, 
                        connectionInfo.getHostname(), 
                        connectionInfo.getDatabase(), 
                        connectionInfo.getUsername(), 
                        connectionInfo.getPassword());

                conn = DriverManager.getConnection(mySQLConnectionString);

            } catch (Exception ex) {
                throw new DatabaseConnectionException(ex);
            }

        }

        return conn;  

    }


    private ConnectionInfo getConnectionInfo() throws IOException {

        try (InputStream input = MySQLDatabase.class.getResourceAsStream(DATABASE_PROPERTIES_FILENAME)) {

            callback.onLoadingProperties();

            Properties prop = new Properties();
            prop.load(input);

            String hostname = prop.getProperty(KEY_HOSTNAME);
            String database = prop.getProperty(KEY_DATABASE);
            String username = prop.getProperty(KEY_USERNAME);
            String password = prop.getProperty(KEY_PASSWORD);

            ConnectionInfo connectionInfo = new ConnectionInfo(hostname, database, username, password);

            return connectionInfo;
        }

    }


    public static class ConsoleOutput implements Callback {

        @Override
        public void onLoadingProperties() {
            System.out.println("Getting properties file...");
        }

        @Override
        public void onConnectingToDatabase() {
            System.out.println("Connecting to database...");
        }

    }


    public static interface Callback {

        void onLoadingProperties();
        void onConnectingToDatabase();

    }


    public static class DatabaseConnectionException extends RuntimeException {

        private static final long serialVersionUID = -1343640756318904341L;

        public DatabaseConnectionException(Exception ex) {
            super(ex);
        }

    }


}
\$\endgroup\$
1
\$\begingroup\$

The original solution from the question, as well as the one suggested by @oopexpert seem too complex and over-engineered.

I guess that the author of the question is trying to implement just a simple wrapper around DriverManager.getConnection(), which reads connection parameters from an expected location.

Some remarks about the original design choices:

  • getConnection() should return a new Connectioneach time. The huge drawback of the original solution is that the returned instance represents a sort of singleton. What happens if this instance is closed somewhere by the caller?

  • I don't suggest to use connection pooling here, e.g. Hikari - let this topic just remain out of scope of this review.

  • There is not enough reason to use stateful objects for the functionality: we need just to provide some additional expected values to the factory method and can remain in static context.

  • The exceptions should be re-thrown, except the I/O that might occur when the expected properties cannot be loaded: this is critical and blocking for the functionality and means a completely unwanted state of the application.

  • Since JDBC4, there is no need to call Class.forName to load the driver. THe driver only needs to be on the classpath.

How it could look like after refactoring:

public class Database {

  private static final String PROPS_FILE_NAME = "database.properties";
  // optionally extract prop keys in constants 

  public static Connection getConnection() throws SQLException {
    Properties properties = readProperties();
    String url = String.format("jdbc:mysql://%1$s/%2$s",
                               properties.getProperty("hostname"),
                               properties.get("database"));
    return DriverManager.getConnection(url, 
                                       properties.getProperty("username"), 
                                       properties.getProperty("password"));
  }

  private static Properties readProperties() {
    try (InputStream input = Database.class.getResourceAsStream(PROPS_FILE_NAME)) {
      Properties props = new Properties();
      props.load(input);
      return props;
    } catch (IOException ex) {
      throw new IllegalStateException(ex);
    }
  }

}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy