Incorrect Use of JDBC Connection Pool

Question

I am running a Spring MVC application, backed by a MySQL database which I am accessing using JDBC. I have been using the same code for a while, and have never really taken a look as to whether or not I was using it correctly(using connection pools properly,etc).

I am aware that there is the JDBCTemplate out there, and I have considered using it, but if the only advantage is that I would just have to not write the boilerplate code for, then I am not quite convinced I should be using it. In fact, I prefer the readability of my code over the JDBCTemplate code.

Below is the code in my DAO, for some reason, I feel like I am not using ConnectionPooling correctly.

public Agent get(Integer id){
    ConnectionPool pool = new ConnectionPool();
    Connection connection = pool.getConnection();
    PreparedStatement ps = null;
    try{
        String query = "SELECT * FROM agent where id= ?";
        ps = connection.prepareStatement(query);
        ps.setInt(1,id);
        ResultSet rs = ps.executeQuery();

        Agent agent = null;
        if(rs.next()){
            agent = new Agent();
            agent.setFirstName(rs.getString(1));
            agent.setLastName(rs.getString(2));
            agent.setStreet(rs.getString(3));
            agent.setCity(rs.getString(4));
            agent.setZip(rs.getString(5));
            agent.setState(rs.getString(6));
            agent.setUsername(rs.getString(7));
            agent.setPassword(rs.getString(8));
            agent.setId(rs.getInt(9));
            agent.setEmail(rs.getString(10));


        }
        return agent;
    }
    catch(SQLException e)
    {
        e.printStackTrace();
        return null;
    }
    finally{
        ConnectionUtility utility = new ConnectionUtility();
        utility.closePreparedStatement(ps);
        pool.freeConnection(connection);
    }

}

The code above is what I am worried about the most being incorrect, but I have a few utility classes that may also be contributing to bad practice/improper code.

Below is my ConnectionUtility class.

public class ConnectionUtility{

public static void closeStatement(Statement s){
    try{
        if(s != null){
            s.close();
        }
    }
    catch(SQLException e){
        e.printStackTrace();
    }
}
public void closePreparedStatement(Statement ps){
    try{
        if(ps != null){
            ps.close();
        }
    }
    catch(SQLException e){
        e.printStackTrace();
    }
}
public static void closeResultSet(ResultSet rs){
    try{
        if(rs != null){
            rs.close();
        }
    }
    catch(SQLException e){

    }
}

}

Here is my ConnectionPool class,

public class ConnectionPool { private static ConnectionPool pool = null;

public ConnectionPool(){
}
public static ConnectionPool getInstance(){
    if(pool == null){
        pool = new ConnectionPool();
    }
    return pool;
}

@SuppressWarnings("static-access")
public Connection getConnection(){
    try{

        return ConnectionFactory.getInstance().getConnection();
    }
    catch(SQLException e){
        e.printStackTrace();
        return null;
    }

}
public void freeConnection(Connection c){
    try{
        c.close();
    }
    catch(SQLException e){
        e.printStackTrace();
    }
}

}

Again, I feel like I am really using all of these classes incorrectly, even though everything is working fine, but nothing has been put to the test in production.

I prefer staying with JDBC, so please no advice on making the switch to another.

Answer 1

There are two problems with your code:

1) You are creating a new ConnectionPool object for each call to get(). Your ConnectionPool class is designed to be a singleton, so enforce it by making the constructor private and change your client code to ConnectionPool pool = ConnectionPool.getInstance().

2) Basically the same issue with ConnectionUtility. You have static methods, but you're using them in a non-static way. Replace

ConnectionUtility utility = new ConnectionUtility();
utility.closePreparedStatement(ps);

with

ConnectionUtility.closePreparedStatement(ps);

and make the ConnectionUtility constructor private and the class final.

Singletons and utility classes can be tricky to get right, so I'd recommend you use static analysis tools like Findbugs to pick up these kinds of problems. It will warn you when you have a class that looks like it should be a singleton or utility class but is not being used in that way.

Answer 2

One of the core principles of Spring is dependency injection. Spring is based around a belief that injecting the components that a class uses into that class leads to easier-to-read, easier-to-test, and easier-to-maintain code over having classes/code (such as your get() method) responsible for finding their own dependencies.

As an example of what this means in more concrete terms: your get() method depends on at least two other classes: 1) the "connection pool" and 2) the connection itself. The get() method has intimate knowledge of how it needs to obtain these instances.

As an alternative to your style of coding here, with the DI approach the class that owns your get() method would have the Connection (or Datasource) injected into it (via a setter or constructor injection).

Now, why does this simple change make code easier and better?

Because the get() method no longer needs to be concerned with details which are not it's core responsibility. The core responsibility of the get() method is to know how to get an Agent given an Integer id. Why does this method also need to know 1) where to get connections from and 2) that you want to pool connections?

What happens when you want to change this connection logic? You need to touch the code of each and every data access method in your application. This would be a far, far harder change than it needs to be.

This is the power of dependency injection: it allows you to change the details (such as where a JDBC connection comes from) without having to also change the code that happens to use these details.

As for your actual connection pool code, it seems like you are misunderstanding two concepts:

1) Your ConnectionPool claims to want to be a Singleton but you expose a public constructor, allowing collaborators to completely default the purpose of having a single instance of ConnectionPool.

2) Your connection pool is not actually a pool of connections! The idea of a connection pool is to open up N connections to a database, and then hand out each of these N connections to code that needs a connection on-demand. The core idea here is that you can recycle connections and avoid the expensive costs of opening a new connection for each request. In a connection pool, when the code using a connection is done with it's connection, the physical connection is not actually terminated - instead, the connection handle is simply returned to the pool to be used again by another request/thread/method.

Most importantly, in applications that use a connection pool the code responsible for data access typically doesn't even know it's connections are being pooled - instead, the DAO simply has a reference to a DataSource interface and the DAO has no idea what actually happens when it asks the DataSource for a connection or what happens when the connection is released. Thus, you can abstract away the details of "how do I connect" from the code responsible for higher-order logic such as "how do I get an Agent from this integer?". This abstraction is what allows you to change one layer of your application without rewriting all of the other layers - you've decoupled the layers and each is only concerned with what it is actually responsible for.

I would strongly, strongly suggest you do some more reading on not just the idea of a connection pool but also Dependency Injection. Why in the world would you use Spring without the DI components? As for connection pools, why spend time re-inventing the wheel by writing your own instead of using a number of already-existing and popular libraries like commons-dbcp or c3p0? Instead of re-inventing the wheel, use an existing library (which is less likely to have bugs than your home-brewed solution) and focus on building your actual application.