2
\$\begingroup\$

I'm making a wrapper for querying the database so that I can more easily control the flow of database management and to make it easier for me to write data beans around my database. This code is going to be used a ton so I'm looking to make it as clean as possible.

/**
 * Query the database.
 * 
 * @param sql
 *            The query statement
 * @return List of rows returned by the query.
 */
public List<Map<String, Object>> query(String sql) {
    return this.query(sql, null);
}

/**
 * Query the database.
 * 
 * @param sql
 *            The query statement
 * @param params
 *            The prepared statement parameters (in order).
 * @return List of rows returned by the query.
 */
public List<Map<String, Object>> query(String sql, Object[] params) {
    final String METHODNAME = "query";
    List<Map<String, Object>> resList = null;

    try {
        if (conn != null && !conn.isClosed()) {
            resList = new ArrayList<Map<String, Object>>();

            PreparedStatement stmt = conn.prepareStatement(sql);
            if (params != null && params.length > 0) {
                for (int i = 1; i <= params.length; i++) { // 1 based, not 0
                    stmt.setObject(i, params[i - 1]);
                }
            }

            ResultSet res = stmt.executeQuery();

            while (res.next()) {
                Map<String, Object> row = new HashMap<String, Object>();

                for (int i = 1; i <= res.getMetaData().getColumnCount(); i++) {
                    String key = res.getMetaData().getColumnName(i);
                    Object val = res.getObject(i);
                    row.put(key, val);
                    resList.add(row);
                }
            }

        } else {
            logger.log(LogLevel.ALL, CLASSNAME, METHODNAME, "Attempted to run query when "
                    + "there was no established connection to the database.");
        }
    } catch (Throwable e) {
        logger.log(LogLevel.ERROR, CLASSNAME, METHODNAME,
                "An error occurred when trying to run the query.");
    }

    return resList;
}
\$\endgroup\$
4
\$\begingroup\$

Resource-leak: you neither close the statement nor the result set. Depending on the database driver, this can lead to system resource leaks, blocked OS handles, etc. Always close database objects ASAP (apart from connections, which you can hold for a longer time.)

Multiple looping over metadata: you reiterate over result set metadata for each row in the result set. As you do not know the underlying implementation, this may be quite an expensive operation, there's no way to be sure. Thus, better do this once and cache the information you need.

Error handling: you catch everything, log it, and let the application continue running without any indication of an error, eventually based on incomplete data. Bad idea. Do not catch exceptions here, declare the method as "throws SQLException" (or maybe re-wrap it in MyOwnApplicationException or whatever) - just don't continue running in error conditions.

Multithreading: connections normally cannot be shared amongst multiple threads (eventually invalidating their underlying objects and doing all kinds of funny things.) I see no precautions to avoid multi-threaded access in the code.

General utility: ask yourself, what are you going to do with this map? Create objects from the contents? Create excel reports? Is your application code written to handle each and every funny datatype a driver implementor may be inclined to return via getObject()? I sincerely doubt this is really useful. For object mapping, have a look at various ORM frameworks / JPA, for reporting there are other frameworks. This looks like the re-invention of a wheel to me.

\$\endgroup\$
  • \$\begingroup\$ Thanks for all the help! I have a question about the try/catches, though. Aren't there some situations in which I would want an exception to degrade gracefully? Or is that logic typically handled through if() blocks and actual exceptions should always thrown? \$\endgroup\$ – jros Feb 22 '17 at 5:24
  • \$\begingroup\$ There are situations, where you can gracefully handle an exception and continue, but this is not one of these cases. What you should never do is, to operate on incomplete data as if it was complete and just go on with the program. Imagine you have a table of 100 invoice items and reading of the 51st fails. Then you will only have 50 items on your invoice, loose half your money, and nobody notices it. One very common pattern is, having a central exception handler "way up" in the application, which catches all and displays a "something went wrong" message to the user. \$\endgroup\$ – mtj Feb 22 '17 at 7:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.