Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Please review this connection pool. I tested it and it works but can we improve it from design or performance perspective?

public class ConnectionPool {

    private static final int MAX_SIZE=10;
    private static final BlockingQueue<Connection> bq;

    static{
        bq= new ArrayBlockingQueue<Connection>(MAX_SIZE);
        for(int i=0;i<MAX_SIZE;i++)
        {
            try {
                bq.add(makeConnection());
            } catch (SQLException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }
        }

        System.out.println("total size:" + bq.size());
    }

    public static Connection getConnection() throws InterruptedException
    {
        System.out.println("size before getting connection"+ bq.size());
        Connection con=bq.take();
        System.out.println("size after getting connection"+ bq.size());
        return (con);
    }

    public static boolean releaseConnection(Connection con) throws InterruptedException
    {
        System.out.println("size before releasing connection"+ bq.size());
        boolean bool =bq.add(con);
        System.out.println("size after releasing connection"+ bq.size());
        return (bool);
    }

    public static Connection makeConnection() throws SQLException {
        Connection conn = null;
        Properties connectionProps = new Properties();
        connectionProps.put("user", "root");
        connectionProps.put("password", "java33");
        try {
            Class.forName("com.mysql.jdbc.Driver");
        } catch (ClassNotFoundException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }  
        conn = DriverManager.getConnection("jdbc:" + "mysql" + "://"
                + "localhost" + ":" + "3306" + "/test", connectionProps);

        System.out.println("Connected to database");
        return conn;
    }
}
share|improve this question
    
Like another poster, I highly suggest bonecp: jolbox.com You should have VERY good reasons to write your own db connection pool. Furthermore, Spring projects.spring.io/spring-framework has many high level object pooling constructs. –  dres Oct 25 '13 at 20:51
add comment

3 Answers 3

Overall:

  • Are you sure that you want to keep a static number of connections? Why not keep a minimum set of connections and create new ones when needed?
  • ConnectionPool shouldn't have static fields as signalpillar said.
  • You don't check if the connections have timed out after a period of wait
  • Loggers are nice -> you don't necessary have to remove all useful debug output when going to production
  • You might want to have some sort of a timeout for the blocking queue depending on the application. Then you would poll rather than take from the queue.

Static initializer:

  • Is it sensible to catch exceptions per makeConnection - There's probably something wrong if any of them fail.
  • You don't check if there really are 10 connections in the pool after initialization.
  • What tt34 said.

Method getConnection:

  • return is not a method, parenthesis are not needed
  • do you really want to throw an InterruptedException from your ConnectionPool?

Method releaseConnection:

  • The same as getConnection

Method makeConnection:

  • this could be perhaps renamed as connect or openConnection
  • It would be better that the connection parameters are given as parameters rather hard-coding - you'll probably just testing at the moment but still
  • You should not just ignore the ClassNotFoundException in Class.forName(...): The program will fail again on the next instruction. Rethrow the exception or wrap it as a run-time exception if you do not want to show the ClassNotFoundException in the method signature
  • Rather than concatenating multiple strings, consider using String.format
share|improve this answer
    
I understand all the suggestions except one. I dont understand why we cant create all the connections in a static block and have static methods to get and release the connections. –  user830818 Aug 19 '11 at 20:21
    
The question is about (re)usability of the class. With static initializers and methods you make a decision that there shall be one and no more than one connection pool. This decision is better left to the client of the pool. Also consider testability - what limitations to testing ensues from static state? Thinking about different possibilities of how, where, when and why pool is used will lead to at least a few more arguments against the static approach. –  Aleksi Yrttiaho Aug 20 '11 at 6:19
add comment

Why do you need to load driver each time you call makeConnection()? I suggest to do it only once per class loading. Consider a case when makeConnection() will throw an exception every time you call it and bq will be empty. I suggest to throw a special exception in that case.

share|improve this answer
add comment

First of all I would suggest you to look at this library DBCP

As for the code you have showed - It will be better to avoid using of static state and behavior in this class. The way to control number of instances of this class can be propagated to the client code using different approaches - injection for instance. As connection data may change better to encapsulate it in some class like Destination

class Destination(userName, password, host, port, dbName)
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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