Java Forum / General / November 2007
Database helper class with PreparedStatements
teser3@hotmail.com - 09 Nov 2007 00:27 GMT I have a JDBC working with Oracle 9i where database is inserted/ updated maybe 10 times at most during a week with very little usage in my Tomcat 4.1.3 Container.
The Database classes I have used for the past year are working great but I wonder if I should be closing my connection in my database helper class with Prepared statements:
public class DbInsert { private PreparedStatement stat; private Connection connection;
public DbInsert(Connection connection) { this.connection = connection; }
public void cityInserter(FormBean city) throws SQLException { stat = connection.prepareStatement("Insert into City (street, school) values (?,?)"); stat.setString(1, city.getStreet()); stat.setString(2, city.getSchool()); stat.executeUpdate(); }
//more Methods with preparedstatements here.... }
....
public class DbWork {
private Connection connection = new ConnectionMgr().getConnection();
public dbMethod(FormBean city) { try { new DbInsert(connection).cityInserter(city); } catch(SQLException ex) { System.out.println(ex); } finally { connection.close(); } }
//more db methods using prepared statements here
..... }
When I experiment and put a close statement in the [b]DbInsert[/b] class method then my database insert wont work because it would be closed when it is called in the [b]DbWork[/b] class?
public void cityInserter(FormBean city) throws SQLException { stat = connection.prepareStatement("Insert into City (street, school) values (?,?)"); stat.setString(1, city.getStreet()); stat.setString(2, city.getSchool()); stat.executeUpdate(); connection.close(); }
Please advise.
Arne Vajhøj - 09 Nov 2007 00:38 GMT > I have a JDBC working with Oracle 9i where database is inserted/ > updated maybe 10 times at most during a week with very little usage in [quoted text clipped - 58 lines] > class method then my database insert wont work because it would be > closed when it is called in the [b]DbWork[/b] class? I am surprised that it even work with a connection.close in dbMethod.
For very small single desktop apps you can consider a single permanent open database connection.
For a complex app or any web app, then you should use a connection pool and only keep the connection open when you need it.
Something like:
public void cityInserter(FormBean city) throws SQLException { Connection connection = new ConnectionMgr().getConnection(); PreparedStatement stat = connection.prepareStatement("Insert into City (street, school) values (?,?)"); stat.setString(1, city.getStreet()); stat.setString(2, city.getSchool()); stat.executeUpdate(); connection.close(); }
Martin Gregorie - 09 Nov 2007 10:52 GMT > The Database classes I have used for the past year are working great > but I wonder if I should be closing my connection in my database > helper class with Prepared statements: I prefer to close a connection in the same class that opens it and at the same logical level within the class, i.e. if you open it in a method then close it in another method belonging to the same class and balance the logical level of the method calls within the invoking class. I don't open connections in a constructor because I don't like constructors to throw exceptions, but ymmv.
As you say this is a rarely used function and is used to run a single INSERT statement, I'd put the connection open and close into a single invoking method so the connection exists for the minimum time.
 Signature martin@ | Martin Gregorie gregorie. | Essex, UK org |
Lew - 09 Nov 2007 13:38 GMT > I don't > open connections in a constructor because I don't like constructors to > throw exceptions, but ymmv. You shouldn't open connections in a constructor because it can cause bugs. Opening a connection is not part of object construction so it doesn't belong in a constructor.
You shouldn't do real work from an unconstructed object. Once the object is completely built it's safe to use.
 Signature Lew
Arne Vajhøj - 12 Nov 2007 02:38 GMT > You shouldn't open connections in a constructor because it can cause > bugs. Opening a connection is not part of object construction so it > doesn't belong in a constructor. If the connection is a field then opening the connection is part of the construction.
Arne
Lew - 12 Nov 2007 05:01 GMT >> You shouldn't open connections in a constructor because it can cause >> bugs. Opening a connection is not part of object construction so it >> doesn't belong in a constructor. > > If the connection is a field then opening the connection > is part of the construction. Except that the OP's code didn't treat the connection that way, but closed it prior to disposal of the object.
I should have said, "Either open the connection in the constructor and *keep it for the object's entire lifetime*, or do not open the connection in the constructor and go ahead and close it before the object dies."
And even if the connection is a field doesn't mean that it *must* open in construction. It depends on the lifetime of the connection relative to the object's lifetime. If the connection lives during the entire life of the object, fine, but that wasn't the case here.
It is not uncommon to have a field represent the connection that is re-used for new connections during the object's lifetime, and therefore not opened during construction.
 Signature Lew
Arne Vajhøj - 12 Nov 2007 02:37 GMT > As you say this is a rarely used function and is used to run a single > INSERT statement, I'd put the connection open and close into a single > invoking method so the connection exists for the minimum time. You could do that for a frequently used method as well.
Allocate a connection from pool and deallocate is nothing compared to the database operation itself.
Arne
Are Nybakk - 09 Nov 2007 14:54 GMT > I have a JDBC working with Oracle 9i where database is inserted/ *snip*
I started replying to this post, but I gave it up. I don't quite get what you want answered here. I'll give you some tips on how to organize things.
Make a class that takes care of the database connection:
public class DatabaseConnector {
private Connection con;
public DatabaseConnector(user, pass, ...) { con = ...; }
public Connection getConnection() { return con; }
public void close() { con.close(); }
//...
}
And a database handler class:
public class DatabaseHandler {
private DatabaseConnector dbCon; private Connection con; private PreparedStatement stmt1 = new PreparedStatement("...");
public DatabaseHandler(user, pass, ...) { dbCon = new DatabaseConnector(user, pass, ...); con = dbCon.getConnection(); }
public void insertSomeObject(SomeObject obj) { stmt.setXxx(...); //...
stmt.executeXxx(); }
/...
public void closeConnection() { dbCon.close(); }
}
Remember that the point of PreparedStatements is a way to declare statements that are frequently used. To declare such a statement for every method call removes the point entirely.
About closing connections, simply close them when you won't be using it for a while. I don't think open connections would make any problem unless the database is accessed by a lot of clients simultaneously.
Are Nybakk - 09 Nov 2007 15:00 GMT >> I have a JDBC working with Oracle 9i where database is inserted/ *snip*
> unless the database is accessed by a lot of clients simultaneously. Oh and by the way; there's a group called comp.lang.java.databases that would fit better than this group.
Lew - 09 Nov 2007 15:10 GMT > Remember that the point of PreparedStatements is a way to declare > statements that are frequently used. To declare such a statement for > every method call removes the point entirely. Not entirely. PreparedStatements also provide a kind of type safety for query parameters, and protect against injection attacks. There's really no reason not to use PreparedStatements routinely.
 Signature Lew
teser3@hotmail.com - 10 Nov 2007 22:21 GMT Thanks for info. I combined the classes and was wondering if this is more efficient?
public class DbWork {
private Connection connection = new ConnectionMgr().getConnection(); private PreparedStatement stat;
public void cityInserter(FormBean city) throws SQLException { stat = connection.prepareStatement("Insert into City (street, school) values (?,?)"); stat.setString(1, city.getStreet()); stat.setString(2, city.getSchool()); stat.executeUpdate(); } .......
public dbMethod(FormBean city) { try { cityInserter(city); } catch(SQLException ex) { System.out.println(ex); } finally { connection.close(); } } ..... }
Lew - 10 Nov 2007 23:04 GMT > Thanks for info. I combined the classes and was wondering if this is > more efficient? [quoted text clipped - 5 lines] > ConnectionMgr().getConnection(); > private PreparedStatement stat; There are consequences to making the connection happen at construction time and making 'connection' and 'stat' instance variables.
> public void cityInserter(FormBean city) throws SQLException > { [quoted text clipped - 5 lines] > } > ....... It's better to leave the sample code compilable than to throw in this kind of ellipsis.
> public dbMethod(FormBean city) You need a return type such as 'void' for a method.
> { > try [quoted text clipped - 8 lines] > { > connection.close(); The problem here is that connection is part of the instance, and created during construction, but you close() it before the object is disposed. This leaves the possibility of client code attempting to re-use the object after the connection has been closed.
> } > } > ...... <http://www.physci.org/codes/sscce.html>
> }
 Signature Lew
teser3@hotmail.com - 10 Nov 2007 23:21 GMT > tes...@hotmail.com wrote: > > Thanks for info. I combined the classes and was wondering if this is [quoted text clipped - 55 lines] > -- > Lew Thanks for your quick response.
Would this be better where I put the Connection and PreparedStatement instances in the method??
public class DbWork { public void cityInserter(FormBean city) throws SQLException { Connection connection = ConnectionMgr().getConnection(); PreparedStatement stat stat = connection.prepareStatement("Insert into City (street, school) values (?,?)"); stat.setString(1, city.getStreet()); stat.setString(2, city.getSchool()); stat.executeUpdate(); }
public void dbMethod(FormBean city) { try { cityInserter(city); } catch(SQLException ex) { System.out.println(ex); } finally { connection.close(); //not sure if I corrected this issue here or not??
> The problem here is that connection is part of the instance, and created > during construction, but you close() it before the object is disposed. This > leaves the possibility of client code attempting to re-use the object after > the connection has been closed. } }
teser3@hotmail.com - 10 Nov 2007 23:26 GMT Thanks for your quick response.
Would this be better where I put the Connection and PreparedStatement instances in the method??
public class DbWork { public void cityInserter(FormBean city) throws SQLException { Connection connection = ConnectionMgr().getConnection(); PreparedStatement stat = connection.prepareStatement("Insert into City (street,
> school) values (?,?)"); stat.setString(1, city.getStreet()); stat.setString(2, city.getSchool()); stat.executeUpdate(); }
public void dbMethod(FormBean city) { try { cityInserter(city); } catch(SQLException ex) { System.out.println(ex); } finally { connection.close(); //not sure if I corrected this issue here or not??
> The problem here is that connection is part of the instance, and created > during construction, but you close() it before the object is disposed. This > leaves the possibility of client code attempting to re-use the object after > the connection has been closed. } }
teser3@hotmail.com - 10 Nov 2007 23:28 GMT Thanks for your quick response.
Would this be better where I put the Connection and PreparedStatement instances in the method??
public class DbWork { public void cityInserter(FormBean city) throws SQLException { Connection connection = ConnectionMgr().getConnection(); PreparedStatement stat = connection.prepareStatement("Insert into City (street, school) values (?,?)"); stat.setString(1, city.getStreet()); stat.setString(2, city.getSchool()); stat.executeUpdate(); }
public void dbMethod(FormBean city) { try { cityInserter(city); } catch(SQLException ex) { System.out.println(ex); } finally { connection.close(); //not sure if I corrected this issue here or not??
> The problem here is that connection is part of the instance, and created > during construction, but you close() it before the object is disposed. This > leaves the possibility of client code attempting to re-use the object after > the connection has been closed. } }
Lew - 11 Nov 2007 01:24 GMT > Thanks for your quick response. > [quoted text clipped - 13 lines] > { > connection.close(); Now the problem is that this variable 'connection' has not been declared. Remember, a variable loses scope with the closing brace. That means that the local variable 'connection' from method cityInserter() is not available any more.
The good new is that you can put the finally block in cityInserter() and get rid of dbMethod().
Try it like this (some class names changed to better reflect their purpose):
public class CityDb { private static final String INSERT_SQL = "INSERT INTO city (street, school) VALUES (?,?)";
private final Logger logger = Logger.getLogger( getClass() );
/** Insert the city into the DB. * @param city <code>City</code> to insert. * @return boolean <code>true</code> iff insert succeeded. */ public City insert( City city ) { List<City> batch = new ArrayList<City>(); batch.add( city ); return insert( batch ); }
/** Insert a batch of cities into the DB. * Uses one <code>PreparedStatement</code> for the whole batch. * @param city <code>City</code> to insert. * @return boolean <code>true</code> iff all inserts succeeded. */ public boolean insert( Iterable <City> batch ) { if ( batch == null ) { throw new IllegalArgumentException( "Batch cannot be null" ); } Connection connection = ConnectionMgr().getConnection(); if ( connection == null ) { return false; } try { return insert( batch, connection ); } finally { try { connection.close(); } catch ( SQLException exc ) { logger.error( "Cannot close. "+ exc.getMessage(); logger.debug( exc ); } } }
/** Insert a batch of cities into the DB. * Assumes a live <code>Connection</code> for the whole batch. * @param city <code>City</code> to insert. * @param connection <code>Connection</code> through which to insert. * @return boolean <code>true</code> iff all inserts succeeded. */ boolean insert( Iterable <City> batch, Connection connection ) { try { connection.setAutoCommit( false );
PreparedStatement stat = connection.prepareStatement( INSERT_SQL ); for ( City city : batch ) { if ( city == null ) { throw new IllegalArgumentException( "City cannot be null" ); } insert( city, stat ); } connection.commit(); return true; } catch ( SQLException ex ) { logger.error( "Whoops! "+ ex.getMessage(); logger.debug( ex ); try { connection.rollback(); } catch ( SQLException exc ) { logger.error( "Cannot rollback. "+ exc.getMessage(); logger.debug( exc ); } return false; } }
private void insert( City city, PreparedStatement stat ) throws SQLException { stat.setString( 1, city.getStreet() ); stat.setString( 2, city.getSchool() ); stat.executeUpdate(); }
}
 Signature Lew
teser3@hotmail.com - 11 Nov 2007 02:03 GMT > tes...@hotmail.com wrote: > > Thanks for your quick response. [quoted text clipped - 131 lines] > > - Show quoted text - Lew, Thanks for all your time and help on this!
Lew - 11 Nov 2007 02:17 GMT > Lew, Thanks for all your time and help on this! Bear in mind that the code I presented is itself untested. Assuming I made no errors, it still is not the only way to go about things.
The decomposition of the methods was designed to allow PreparedStatement reuse where possible. It also allows sharing a Connection between helper classes in some putative expansion of the concept. That's why all the overloads.
If you parametrize the class on the entity type rather than just City, you can come up with a pretty decent Data Access superclass. You still need to override methods in child classes that are smart about object<->PreparedStatement and object<->ResultSet specifics.
When I've done this I wind up with a fairly complex framework, one really big AbstractDataAccessor class and a lot of very easy, quick child classes that correspond to each entity. Once the framework is in place it's easy to adapt to new entities.
That said, the new Java Persistence API (JPA) (see also Hibernate) looks to be a much more robust, portable way to handle object persistence to RDBMSes.
 Signature Lew
Arne Vajhøj - 12 Nov 2007 02:48 GMT > I started replying to this post, but I gave it up. I don't quite get > what you want answered here. I'll give you some tips on how to organize [quoted text clipped - 21 lines] > > } What does that class provide that Connection does not ?
> And a database handler class: > [quoted text clipped - 23 lines] > > } That DatabaseConnector is not needed is emphasized by the fact that you have both the DatabaseConnector wrapper and the underlying Connection as fields here.
Furthermore you should really open and close connection in the insert method. If you keep the DatabaseHandler around then you collect way to many database connections. If you construct and close DatabaseHandler for every insert call, then you could just as well do everything in the insert method.
> Remember that the point of PreparedStatements is a way to declare > statements that are frequently used. To declare such a statement for > every method call removes the point entirely. PreparedStatement provides other useful functionality including: - handling of special characters like ' (both relevant for scottish names and malicious SQL injection) - date format handling
> About closing connections, simply close them when you won't be using it > for a while. I don't think open connections would make any problem > unless the database is accessed by a lot of clients simultaneously. If it does not cost anything to make the code scalable, then why' not do it ?
You never know how long time some code will live and in how many different contexts it will be used !
Arne
Are Nybakk - 12 Nov 2007 14:12 GMT >> I started replying to this post, but I gave it up. I don't quite get >> what you want answered here. I'll give you some tips on how to [quoted text clipped - 23 lines] > > What does that class provide that Connection does not ? I see your point. The idea is to isolate what has got to do with the actual initialization and maintainance of the server connection. Only this class needs to know about JDBC driver and such. That way it can also be reused.
>> And a database handler class: >> [quoted text clipped - 27 lines] > that you have both the DatabaseConnector wrapper and the underlying > Connection as fields here. Alright let me revise. I guess I wrote this in a hurry.
(Error catching left out)
public class DatabaseConnector {
private String serverURL = "..."; private String driverName = "..."; private Connection con; private PreparedStatement stmt1 = new PreparedStatement("..."); //...
public DatabaseConnector(user, pass) { Class.forName(driverName); con = DriverManager.getConnection(serverURL, user, pass); }
public void insertSomeObject(SomeObject obj) { stmt.setXxx(obj.getZzz); //...
stmt.executeWww(); }
public boolean isActive() { return !con.isClosed(); }
public void close() { stmt.close(); con.close(); }
//...
}
public class DatabaseHandler {
private DatabaseConnector dbCon;
public DatabaseHandler(user, pass, ...) { dbCon = new DatabaseConnector(user, pass, ...); }
public insert(SomeObject obj) { //dbCon.insertSomeObj(...) }
public insert(Collection<SomeObject> obj) { //loop: dbCon.insertSomeObj(...) }
//...
public void end() { if(dbCon.isActive()) { dbCon.close(); } //Any other close operataions. }
}
> Furthermore you should really open and close connection in the > insert method. If you keep the DatabaseHandler around then you [quoted text clipped - 20 lines] > You never know how long time some code will live and in how many > different contexts it will be used ! Point taken.
> Arne Lew - 12 Nov 2007 14:24 GMT >>> I started replying to this post, but I gave it up. I don't quite get >>> what you want answered here. I'll give you some tips on how to [quoted text clipped - 68 lines] > > private String serverURL = "..."; If you're going to hard-code these values, make them static final Strings (and therefore use all upper case in their names).
> private String driverName = "..."; > private Connection con; [quoted text clipped - 5 lines] > con = DriverManager.getConnection(serverURL, user, pass); > } You don't need to load the driver class but once, not every time you make an object. You could do it in a static initializer for the class (assuming you followed my advice to make the driver name a static variable, too).
 Signature Lew
Are Nybakk - 13 Nov 2007 19:59 GMT >>>> I started replying to this post, but I gave it up. I don't quite get *snip*
>> private String serverURL = "..."; > [quoted text clipped - 15 lines] > (assuming you followed my advice to make the driver name a static > variable, too). Hmm, good idea.
Are Nybakk - 12 Nov 2007 14:28 GMT >>> I started replying to this post, but I gave it up. I don't quite get >>> what you want answered here. I'll give you some tips on how to [quoted text clipped - 84 lines] > stmt.executeWww(); > } Commenting myself again =) This makes it hard to reuse the class. It would be better to have a getPreparedStatement and let DatabaseHandler take care of the actual queries.
> public boolean isActive() { > return !con.isClosed(); [quoted text clipped - 64 lines] > >> Arne Are Nybakk - 14 Nov 2007 11:43 GMT >> I started replying to this post, but I gave it up. I don't quite get >> what you want answered here. I'll give you some tips on how to [quoted text clipped - 23 lines] > > What does that class provide that Connection does not ? Let me quote from a thread in comp.lang.java.databases:
"Lew wrote:
>> Which is worse, to take a while to construct a connection, or to run out of connections because your objects never let them go?
David Harper wrote:
> Better *not* to let each object create its own connection. That way, madness lies. As you say, this argues strongly for a connection pool.
The application code doesn't change when you switch to a connection pool. The semantics of the close() call change to "release to the pool" instead of "destroy"."
Now I better understand why I've learned to do it this way. This wrapper class is a connection pool of sorts. One with only one connection.
The pool could contain more connections if needed (several databases perhaps) and a time-out for closing unused, open connections. It should also have a way of "locking" the connection.
>> And a database handler class: >> [quoted text clipped - 33 lines] > close DatabaseHandler for every insert call, then you could > just as well do everything in the insert method. Then rather, get a reference to a connection from the pool and "free" it afterwards.
>> Remember that the point of PreparedStatements is a way to declare >> statements that are frequently used. To declare such a statement for [quoted text clipped - 16 lines] > > Arne
Free MagazinesGet these publications absolutely FREE for up to 12 months. There are no hidden fees and no obligation. Simply choose a title, complete the application form and submit it. Read more ...
|
|
|