2
\$\begingroup\$

I'm creating a plugin for the game Minecraft, and with it I'm using HikariCP and MySQL to store data between server restarts. I've created working code but I'm wanting to improve it.

While the code works fine, I feel there's a simpler way to structure it. I appreciate any suggestions.

HikariFacade.java

public class HikariFacade {

private final HikariConfig hikariConfig;
private final HikariDataSource hikariDataSource;

public HikariFacade(HikariConfig hikariConfig) {
    this.hikariConfig = hikariConfig;
    hikariDataSource = new HikariDataSource(hikariConfig);
}

/**
 * Get a connection from the DataSource
 *
 * @return A connection, or null if one couldn't be obtained
 */
public Connection getConnection() {
    Connection connection = null;
    try {
        connection = hikariDataSource.getConnection();
    } catch (SQLException e) {
        e.printStackTrace();
    }
    return connection;
}

/**
 * Create a PreparedStatement
 *
 * @param sql The SQL for the PreparedStatment
 * @return A prepared statement
 * @throws SQLException
 */
public PreparedStatement createPreparedStatement(String sql) throws SQLException {
    return getConnection().prepareStatement(sql);
}

/**
 * Run an update, then close the resources
 *
 * @param ps Run a PreparedStatement as an update
 * @return The number of rows effected
 * @throws SQLException
 */
public int update(PreparedStatement ps) throws SQLException {
    int i = ps.executeUpdate();
    close(ps.getConnection(), ps, null);
    return i;
}

/**
 * Run a query, then close the resources
 *
 * @param ps Run a PreparedStatement as a query
 * @return The ResultSet from the query
 * @throws SQLException
 */
public ResultSet query(PreparedStatement ps) throws SQLException {
    ResultSet rs = ps.executeQuery();
    close(ps.getConnection(), ps, rs);
    return rs;
}

/**
 * Close a set of database related objects
 *
 * @param connection A Connection to close
 * @param statement  A Statement to close
 * @param resultSet  A ResultSet to close
 */
public static void close(Connection connection, Statement statement, ResultSet resultSet) {
    try {
        if (connection != null) {
            connection.close();
        }
        if (statement != null) {
            statement.close();
        }
        if (resultSet != null) {
            resultSet.close();
        }
    } catch (SQLException e) {
        e.printStackTrace();
    }
}

public HikariDataSource getHikariDataSource() {
    return hikariDataSource;
}
}

DatabaseManager.java

public abstract class DatabaseManager {

protected HikariFacade hikariFacade;

public DatabaseManager(HikariFacade hikariFacade) {
    this.hikariFacade = hikariFacade;
}

}

TownDatabaseManager.java

public class TownDatabaseManager extends DatabaseManager {

public TownDatabaseManager(HikariFacade hikariFacade) {
    super(hikariFacade);
}

public int create(Town town) {
    String sql = "INSERT INTO towns VALUES (?);";
    try {
        PreparedStatement ps = hikariFacade.createPreparedStatement(sql);
        ps.setString(1, String.valueOf(town.getUuid()));
        return hikariFacade.update(ps);
    } catch (SQLException e) {
        e.printStackTrace();
    }
    return 0;
}

public List<Town> loadTowns() {
    String sql = "SELECT * FROM towns;";
    try {
        PreparedStatement ps = hikariFacade.createPreparedStatement(sql);
        ResultSet rs = hikariFacade.query(ps);
        List<Town> towns = new ArrayList<>();
        while (rs.next()) {
            UUID townUuid = UUID.fromString(rs.getString("town_uuid"));
            towns.add(new Town(townUuid));
        }
        return towns;
    } catch (SQLException e) {
        e.printStackTrace();
    }
    return null;
}

}

Here's how this is all used in the end:

Main.java

private void testDb() {
    Town newTown = new Town(UUID.randomUUID());

    int i = new TownDatabaseManager(getHikariFacade()).create(newTown);

    new TownDatabaseManager(getHikariFacade()).loadTowns();

}
\$\endgroup\$
2
\$\begingroup\$

Something which occurs to me is that catching the exception and not rethrowing is a very bad practice. Printing the stack trace is helpful, it tells the consumer of your API that something went wrong, but it is inflexible (it cannot be redirected to another IO device) and, more importantly the failure itself is swallowed. The program keeps on running after an exception, but there is no fallback behavior and the situation seems critical. Consider rethrowing the exception or, better yet, removing the try...catch construct entirely.

\$\endgroup\$
  • \$\begingroup\$ I know it's a bad reason, but I put them there simply to make using the methods less cluttered. \$\endgroup\$ – Jerred Shepherd Dec 17 '16 at 4:35
  • \$\begingroup\$ Do you mean to avoid the throws clause? \$\endgroup\$ – Aluan Haddad Dec 17 '16 at 5:21
  • \$\begingroup\$ No, I meant that whenever I call a method, I'd have to add the try...catch block on it. It's cleaner to have it once in the method itself. \$\endgroup\$ – Jerred Shepherd Dec 17 '16 at 5:44
  • \$\begingroup\$ Why not let the exception bubble up if you don't want to handle it? I don't know much about Minecraft so maybe it's an environmental constraint but generally this seems like a bad idea and a bad trade-off. To quote a famous language designer "it's funny how people think the important thing about exceptions and handling them. That is not the important thing about exceptions!" \$\endgroup\$ – Aluan Haddad Dec 17 '16 at 5:48

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.