Accessing the only element of Java ResultSet

Question

Does this code look OK?

public Map<String, String> getById(long id) {
 String sql = "select * from " + this._TABLE_NAME + " where id = ?";
 PreparedStatement p;
 Map<String, String> result = new HashMap<String, String>();
 try {
 p = conn.prepareStatement(sql);
 p.setLong(1, id);
 ResultSet rs = p.executeQuery();
 ResultSetMetaData rsmd = rs.getMetaData();
 int fieldsCount = rsmd.getColumnCount();
 // is this ok?
 rs.next();
 for (int i=1;i<fieldsCount+1;i++) {
 String cName = rsmd.getColumnName(i);
 result.put(cName, rs.getString(cName));
 }
 } catch (SQLException e) {
 e.printStackTrace();
 }
 return result;
}

Answer 1

Here is a bit reworked version of you code. Some explanation:

• Assume _TABLE_NAME is a constant, so there is no need to build query in every method call.
• Force getting connection and close resources to avoid side effects.
• Propagate SQLException to caller (caller should decide what to do with thrown exception).

Code:

private final static String GET_DATA_BY_ID = "select * from " + _TABLE_NAME + " where id = ?";
public Map<String, String> getById(long id) throws SQLException {
 Connection conn = null;
 Map<String, String> result = new HashMap<String, String>();
 try {
 conn = getConnection();
 PreparedStatement preparedStatement = null;
 ResultSet rs = null;
 try {
 preparedStatement = conn.prepareStatement(GET_DATA_BY_ID);
 preparedStatement.setLong(1, id);
 try {
 rs = preparedStatement.executeQuery();
 if (rs.next()) {
 ResultSetMetaData rsmd = rs.getMetaData();
 int fieldsCount = rsmd.getColumnCount();
 for (int i = 1; i < fieldsCount + 1; i++) {
 String cName = rsmd.getColumnName(i);
 result.put(cName, rs.getString(cName));
 }
 }
 } finally {
 if (rs != null)
 rs.close();
 }
 } finally {
 if (preparedStatement != null)
 preparedStatement.close();
 }
 } finally {
 if (conn != null)
 conn.close();
 }
 return result;
}

• laz: I'd define the Connection, PreparedStatement, and ResultSet all in the same scope before the first try so that they can be closed in the same finally block, avoiding all of that nesting and having only one finally. You already have null checks prior to calling close() so it would work fine.

Answer 2

Sergey's answer highlights the importance of closing the resources - if you omit that, you'll have a connection leak, which means that the database will run out of connections. In a long-running application this is a critical bug, so the ugly try-finally-approach is fully justified. (You could actually hide it by wrapping it in an utility class.)

Checking the ResultSet and PreparedStatement obtained from the JDBC implementation for null is not necessary, since if e.g. obtaining the result set by rs = preparedStatement.executeQuery() did fail already, that means it cannot be closed anyway.

However, I would not throw the raw SQLException to the client, but wrap them in an application specific exception. This allows you to treat different error conditions differently, and decouples your calling code from the SQL layer, which is useful should you want to switch to another persistence mechanism later. (Also, it is very simple.)

private final static String GET_DATA_BY_ID = 
 "select * from " + _TABLE_NAME + " where id = ?";
public Map<String, String> getById(long id) throws YourSpecificPersistenceException {
 final Map<String, String> result;
 try {
 Connection conn = getConnection();
 try {
 PreparedStatement preparedStatement = 
 conn.prepareStatement(GET_DATA_BY_ID);
 try {
 preparedStatement.setLong(1, id);
 ResultSet rs = preparedStatement.executeQuery();
 try {
 if (rs.next()) {
 ResultSetMetaData rsmd = rs.getMetaData();
 int fieldsCount = rsmd.getColumnCount();
 result = new HashMap<String, String>(fieldsCount);
 for (int i = 1; i < fieldsCount + 1; i++) {
 String cName = rsmd.getColumnName(i);
 result.put(cName, rs.getString(cName));
 }
 } else {
 result = Collections.emptyMap();
 }
 } finally {
 rs.close();
 }
 } finally {
 preparedStatement.close();
 }
 } finally {
 conn.close();
 }
 } catch (SQLException e) {
 throw new YourSpecificPersistenceException("Unable to execute statement: '"
 + GET_DATA_BY_ID + "' with parameter [" + id + "]", e);
 }
 return result;
}

Answer 3

You don't need check if (rs != null) and if (preparedStatement != null), because prepareStatement() and executeQuery() always return non-null or fails. Just declare these objects without initialization and close them after. Same for connection probably.

Answer 4

Besides the things that others have already mentioned, I have a couple of things to say:

• Variable naming: p is way too short for a variable name. Consider preStmt, stmt or statement instead. (I commonly use stmt myself) Also, the name _TABLE_NAME does not adhere to the Java naming conventions. If it is a field in the class, then use lower case naming without an underscore at the beginning, like tableName.
• To make it more clear what you are doing (that you are not iterating over the ResultSet but only returning the first and only row), call rs.first() instead of rs.next().
• Instead of first getting the column name from the meta data and then using getString(columnName), there is a getString(int) method that takes the column index as an argument. No need to get the column name when you know the column index.
• Your PreparedStatement variable should be declared inside the try, since it is not needed outside it. This also applies to your String sql variable.

Other than these things, I agree with StitzL's answer

Answer 5

Why not just do it like:

while(rs.next()) {
 result.put(....);
 return result;
}

You can also wrap it in a try-catch-finally.

Also, the use of prepared statement may not be justified in this example, since you use it only once.

• Malachi: please give more explanation. it makes for a better review
• Simon Forsberg: No need to call while(rs.next()) when it would only happen once.
• Simon Forsberg: Using a prepared statement is perfectly OK, no matter if it's used only once or 100 times. It removes any possible vulnerabilities of SQL injection.