0

I am doing code review, and in many cases I found result is not closed if SQL statement returns the records. The business rule for SQL statement is consider only the first record.

Below is the code where I am confused, Why is it returning the value without closing resultset and statement? Is this correct way?

if (rs.next()){
        return new Long(rs.getLong(1));
}

Below is sample code:

private static Long <MethodName>
     (
         oracle.sql.NUMBER[] o_errorCode,
         oracle.sql.CHAR[] o_errorText,
         Long portA,
         Long portZ) throws SQLException
     {
         String errorMessage = "";
         Long[] NROLE = new Long[1];
         Long[] pTask = new Long[1];
         Long dimObject = null;
         Long objectIDA = null;
         Long objectIDZ = null;
         Long relation = null;
         Connection tmpConn = null;
         Statement stmt = null;
         ResultSet rs = null;
         String SQL = null;
         try
         {
             // Retrieve the Epipe circuits that are on the specified ports
             stmt = DbUtil.getConn().createStatement();
             String query = "Select * from Circuit where ... Order by Id";
                 rs = stmt.executeQuery(query);
              if (rs.next()){
                     return new Long(rs.getLong(1));
                }
                 rs.close();
                 stmt.close();
             return null;
         }
         catch (SQLException ex)
         {
             o_errorCode[0] = new oracle.sql.NUMBER(1);
             o_errorText[0] = new oracle.sql.CHAR("SQLException - " + ex.getMessage(),
             oracle.sql.CharacterSet.make(oracle.sql.CharacterSet.DEFAULT_CHARSET));
             return(null);
         }
         catch (Exception e)
         {
             o_errorCode[0] = new oracle.sql.NUMBER(1);
             o_errorText[0] = new oracle.sql.CHAR("Exception - " + e.getMessage(), oracle.sql.CharacterSet.make(
             oracle.sql.CharacterSet.
             DEFAULT_CHARSET));
             return(null);
         }
}
Tom Zych
  • 12,329
  • 9
  • 33
  • 51

3 Answers3

2

Not correct, and since Java 7 easily resolved when using try-with-resources:

try (ResultSet rs = statement.executeQuery()) {
    if ...
        return ...
}

That if rs.next() might do with a better SQL: a kind of limit or a MIN(Id) of a kind.

It is not good style too:

  1. Use a consistent style: SELECT FROM WHERE ORDER BY or select from where by.
  2. Do not use database specific classes; JDBC took much effort to not need it.
  3. I almost suspect that here a PreparedStatement could have been used. Aside from preventing SQL injection it escapes parameters nicely.
Community
  • 1
  • 1
Joop Eggen
  • 96,344
  • 7
  • 73
  • 121
0

Use finally.

    try {
            // Retrieve the Epipe circuits that are on the specified ports
            stmt = DbUtil.getConn().createStatement();
            String query = "Select * from Circuit where ... Order by Id";
            rs = stmt.executeQuery(query);
            if (rs.next()) {
                return new Long(rs.getLong(1));
            }

            return null;
        } catch (SQLException ex) {

        } catch (Exception e) {

        } finally {
            rs.close();
            stmt.close();
        }
Nikhil Talreja
  • 2,691
  • 1
  • 12
  • 20
0

The correct place to close database resources like result set, statement or connection is the finally block of a try-catch statement. Because, JVM visits this block in any condition.

The following usage is safe:

ResultSet rs = null;
Statement stmt = null;
Connection conn = null;
try {
    conn = SomeDBUtility.getConnection();
    stmt = conn.createStatement();
    rs = stmt.executeQuery("<Your SQL string>");
} catch (SQLException e) {
    // handle the exception
} finally {
    rs.close();
    stmt.close();
    conn.close();
}
ovunccetin
  • 7,673
  • 4
  • 38
  • 45