Database helper class with PreparedStatements

[teser3]

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 DbInsert
class method then my database insert wont work because it would be
closed when it is called in the DbWork 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.

 

[=?ISO-8859-1?Q?Arne_Vajh=F8j?=]

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 DbInsert
class method then my database insert wont work because it would be
closed when it is called in the DbWork 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]

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.

 

[Lew]

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.

 

[Are Nybakk]

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]

*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]

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.

 

[teser3]

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]

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;

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
{
stat = connection.prepareStatement("Insert into City (street,
school) values (?,?)");
stat.setString(1, city.getStreet());
stat.setString(2, city.getSchool());
stat.executeUpdate();
}
.......

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
{
cityInserter(city);
}
catch(SQLException ex)
{
System.out.println(ex);
}
finally
{
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.

 

[teser3]

There are consequences to making the connection happen at construction time
and making 'connection' and 'stat' instance variables.


It's better to leave the sample code compilable than to throw in this kind of
ellipsis.


You need a return type such as 'void' for a method.


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.

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]

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]

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]

Thanks for your quick response.

Would this be better where I put the Connection and PreparedStatement
instances in the method??
public void dbMethod(FormBean city)
{
try
{
cityInserter(city);
}
catch(SQLException ex)
{
System.out.println(ex);
}
finally
{
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();
}

}

 

[teser3]

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

}

Lew, Thanks for all your time and help on this!

 

[Lew]

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.

 

[=?ISO-8859-1?Q?Arne_Vajh=F8j?=]

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

 

[Guest]

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

 

[=?ISO-8859-1?Q?Arne_Vajh=F8j?=]

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

//...

}

What does that class provide that Connection does not ?

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

}

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

 

[Lew]

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.

 

[Are Nybakk]

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.

That DatabaseConnector is not needed is emphasized by the fact
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
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.


PreparedStatement provides other useful functionality including:
- handling of special characters like ' (both relevant for
scottish names and malicious SQL injection)
- date format handling


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 !

Point taken.