I am wondering what is the best practice for the following code snippet, do I have to close all stmt and rs everytime the executeQuery is done inside each 'if' or can I do just like now, close rs a开发者_高级运维nd stmt at the end of runOnSqlServer, and close connection at the end fo run method? Thanks for any pointers!
public void runOnSqlServer(Connection con, String[] params, String db){
try{
Statement stmt = con.createStatement();
ResultSet rs = null;
if(isVer){
rs = stmt.executeQuery(micro_verSql);
commonAct(rs, getParameter("isVer"), 1);
}
if(isInfo){
rs = stmt.executeQuery("SELECT DATABASEPROPERTYEX('"+db+"', 'COLLATION')");
commonAct(rs, getParameter("isInfo"), 1);
}
}catch(SQLException){
.....
}finally{
stmt.close();
rs.close();
}
}
public void run(CommandContext ctx) {
try{
...
runOnSqlServer(con, params, sqldb);
...
}catch(Exception ex){
}finally{
if (con != null) con.close();
}
}
IMHO, you should close the statement and result set after the method has completed (in the finally) but close the ResultSet
everytime you re-user it also you should ALWAYS close the connect when you are finished with it.
edit: re-initialise
ResultSet set = statement.executeQuery();
set = statement2.executeQuery();
you are setting the ResultSet "set" to a new result set. The original set is now not pointing to anything but still open waiting to be collected.
You should close all database resources:
- in the method scope in which they were created.
- in reverse order of creation
- in the finally block
- individually wrapped in try/catch blocks.
I think you should consider reading this articles under "Best practices to improve performance in JDBC".
- Optimization with Connection
- Optimization with Statement
- Optimization with ResultSet
As of Java 7 you can use try-with-resources, which clearly marks the scope of your Connection
, Statement
and ResultSet
and automatically closes them. Your method runOnSqlServer(…)
could look like this:
public void runOnSqlServer(Connection con, String[] params, String db){
try (Statement stmt = con.createStatement()) {
if(isVer){
try (ResultSet rs = stmt.executeQuery(micro_verSql)) {
commonAct(rs, getParameter("isVer"), 1);
} // rs is closed here
}
if(isInfo){
try (ResultSet rs = stmt.executeQuery("SELECT DATABASEPROPERTYEX('"+db+"', 'COLLATION')")) {
commonAct(rs, getParameter("isInfo"), 1);
} // rs is closed here
}
} // stmt is closed here
catch(SQLException){
.....
}
// finally not necessary, as rs and stmt are closed automatically
}
As a side note: Don't use constructs like "SELECT DATABASEPROPERTYEX('"+db+"', 'COLLATION')"
unless you are sure db
is not derived from user input. Concatenating strings to build a SQL statement is vulnerable to SQL injection. Use PreparedStatement
instead.
As others have mentioned, using finally
to ensure items are closed is very important.
Also, consider using a connection pool to mitigate the cost of frequently opening and closing connections. c3p0 is one such package.
精彩评论