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;
    }

Answer 1

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.

Answer 2

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.

Answer 3

... 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 ;)

Answer 4

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.