java data access: is this good style of java data access code, or is it too much try finally?

Question

Is this good style of java data access code, or is it too much try finally ?

public List<Item> getItems() throws ItemException {
    ArrayList<Item> items = new ArrayList<Item>();
    try {
        Connection con = ds.getConnection();
        try {
            PreparedStatement pStmt = con.prepareStatement("SELECT ....");
            try {
                ResultSet rs = pStmt.executeQuery();
                try {
                    while (rs.next()) {
                        Item item = new Item();
                        item.setItemNo(rs.getString("item_id"));
                        // ...
                        items.add(item);
                    }
                } finally {
                    rs.close();
                }
            } finally {
                pStmt.close();
            }
        } finally {
            con.close();
        }
    } catch (SQLException e) {
        throw new ItemException(e);
    }
    return items;
}

`ArrayList items = new ArrayList();` should be `List items = new ArrayList();` — Mr_and_Mrs_D, May 02 '13 at 21:56
i would suggest you to go for hibernate. It has hell lot of features. Aarons code look good but if you are using JDBC every time. You will end up creating lot of connection to the database which may take a toll on the applications performance. — akellakarthik, Dec 21 '09 at 08:53

Michael Borgwardt · Answer 1 · 2009-12-21T10:55:45.643

2

According to the API doc, Statements and ResultSets are implicitly closed with the Connection, so yes - 2 of those try/finally-blocks are unnecessary (and very ugly). Explicitly closing Statements may be useful when you're keeping a single Connection around for a lot of queries, in order to reduce memory usage.

edited Dec 21 '09 at 10:55

answered Dec 21 '09 at 08:38

Michael Borgwardt

342,105
78
482
720

In anything but the most simple code, you should close them manually or they will hang around. – Aaron Digulla Dec 21 '09 at 08:43
1

No. If you make sure the Connection is closed, all that might hang around are the objects themselves, and closing them manually will not prevent that either. – Michael Borgwardt Dec 21 '09 at 10:58

Aaron Digulla · Accepted Answer · 2009-12-21T10:29:16.903

Compare it to my code:

Connection con = null;
PreparedStatement pStmt = null;
ResultSet rs = null;
try
{
    con = ds.getConnection();
    pStmt = con.prepareStatement("SELECT ....");
    rs = pStmt.executeQuery();
    while (rs.next()) {
        Item item = new Item();

        item.setItemNo(rs.getString("item_id"));
        ...

        items.add(item);
    }
}
finally {
    rs = DBUtil.close (rs);
    pStmt = DBUtil.close (rs);
    con = DBUtil.close (rs);
}

Here is what close() looks like:

public static ResultSet close (ResultSet rs) {
    try {
        if (rs != null)
            rs.close ();
    } catch (SQLException e) {
        e.printStackTrace (); 
        // Or use your favorite logging framework.
        // DO NOT THROW THIS EXCEPTION OR IT WILL
        // HIDE EXCEPTIONS IN THE CALLING METHOD!!
    }
    return null; // Make sure no one uses this anymore
}

[EDIT] You'll need to copy this code for the other types.

I also moved all this into a helper class called DBOp so I just have to override processRow(ResultSet row) to do the actual processing and I can omit all that boilerplate code. In Java 5, the constructor of DBOp reads:

public DBOp (Logger log, DataSource ds, String sql, Object... param)

I'm passing in the logger so I can show which instance is actually polling data.

I think you forgot to change the parameter to close() in the second to calls. — Chad Okere, Dec 21 '09 at 09:22
Yeah, you'll need to make one copy of this code per JDBC type :( I wish Sun would have made them implement `Closeable`. — Aaron Digulla, Dec 21 '09 at 10:30
If getConnection() obtains a Connection from a pool, then it should not be closed. Also, I think closing the statement automatically closes the ResultSet too. — Adriaan Koster, Dec 21 '09 at 11:35
@Adriann: This depends on the pool implementation. Those I use will return a connection to the pool when I close it. This way, my code is the same no matter whether I use pooling or not. — Aaron Digulla, Dec 21 '09 at 12:33

score 1 · Answer 3 · answered Dec 21 '09 at 08:51

1

... and you should not close the connection in that getItems() method. It looks like, you stored your Connection object in that ds object. So you should leave it to ds to close it. Otherwise, there is a risk that ds returns an already closed connection with the getConnection() method, and I assume, that's an unwanted and unexpected behaviour ;)

answered Dec 21 '09 at 08:51

Andreas Dolk

113,398
19
180
268

... unless the `ds` object happens to be a `DataSource`, of course. Then you'll have to close the connection when you are done with it. http://java.sun.com/javase/6/docs/api/javax/sql/DataSource.html – Dirk Dec 21 '09 at 09:26
@Dirk, good catch, and the (too) short variable name is a hint that you're right. I thought of the traditional DriverManager where you get the connection through in a static method. – Andreas Dolk Dec 21 '09 at 10:09

java data access: is this good style of java data access code, or is it too much try finally?

3 Answers3

Linked