patternjavaMinor
Returning a database connection
Viewed 0 times
databasereturningconnection
Problem
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
Although I am not really sure what should happen if the properties file doesn't exist, or the MySQL connection fails, should I return
```
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 +
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 +
Solution
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
Some remarks about the original design choices:
-
-
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
How it could look like after refactoring:
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);
}
}
}Code Snippets
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);
}
}
}Context
StackExchange Code Review Q#155269, answer score: 2
Revisions (0)
No revisions yet.