CODEWITHSUNDEEP
javaautocloseable
6harat
6harat

Reputation: 622

Should closing a resource always be a responsibility of the function that opens it?

public ResultSet executeQuery(String query) {
    return session.call(query)
}


public List<Record> findRecords(String name) {
    String query = prepareQuery(name);
    return mapResultToRecord(executeQuery(query));
}

public List<Record> mapResultToRecord(ResultSet rs) {
    try {
        List<Record> list = new ArrayList<>();
        while(rs.hasNext()) {
            list.add(new Record(rs.next()));
        }
    } catch (Exception e) {
        logger.warn("exception while iterating over the recall set", e);
    } finally {
        try {
            rs.close();
        } catch (Exception ex) {
            logger.warn("exception while closing the result set", ex);
        }
    }
}

In the above code the ResultSet was opened by executeQuery but is being closed by mapResultToRecord. Is that the ideal place to close it or should the responsibility be taken up by the findRecords function instead?

Upvotes: 3

Views: 499

Answers (2)

Nathan Hughes
Nathan Hughes

Reputation: 96454

If you open it, close it. Whenever different code is responsible for closing a resource than is responsible for opening it, it's very hard to make sure the resource gets closed in all cases. If the same code opens and closes, it can use try-with-resources to make sure things get closed.

It would be an improvement for the executeQuery method to pass the mapper in as a callback. That is what Spring-jdbc does, it defines an interface, RowMapper, with this method:

T mapRow(ResultSet rs, int rowNum) throws SQLException

which is passed into query methods of the JdbcTemplate. The mapper is responsible solely for using a ResultSet row to populate an object and is not involved in closing anything. If the mapper's responsibility is limited to mapping a single row, and we can pass in the rownumber, then the same mapper can be used for looking up an individual row and for getting multiple rows.

Meanwhile the query-executing code becomes very general purpose, the query and the mapper are the only parts specific to a particular query, and they are both passed in from outside.

The findRecords method should execute the query and call the mapper to get the results from within a try-block that closes the resultSet, maybe like this:

List<T> findRows(String queryName, RowMapper<T> mapper) throws SQLException {
    List<T> list = new ArrayList<>();
    String sql = prepareQuery(queryName);
    try (ResultSet resultSet = session.call(sql);) {
        for (int i = 1; resultSet.hasNext(); i += 1) {
            list.add(mapper.mapRow(resultSet, i));
        }
    }
    return list;
}

There isn't any detail given about what session.call does. It would probably be good for this code to be given the PreparedStatement as well so that it can make sure it gets closed.

Upvotes: 1

CryptoFool
CryptoFool

Reputation: 23139

My short answer is "When possible, Yes".

I think it makes more sense to close the ResultSet in findRecords . It has to be understood that executeQuery returns a resource that has to be closed. There's no way around that. Since it is findRecords that calls executeQuery, I think it's cleaner to have it be responsible for closing the ResultSet that comes back from doing so, in fitting with the idea behind your question.

A general rule is to have each function have a single purpose as much as possible. findRecords's purpose is to utilize a query to retrieve and process some data. mapResultToRecord's purpose is to create a list of records by pulling them from a ResultSet. This all reads very nicely:

public List<Record> findRecords(String name) {
    String query = prepareQuery(name);
    ResultSet resultSet = executeQuery(query);
    List<Record> result = mapResultToRecord(resultSet);
    resultSet.close();
    return result;
}

Look at it the other way (the way you have it). findRecords's purpose it to prepare and execute a query, then hand it off to mapResultsToRecord for continued processing. mapResultsToRecord's purpose is to create a list of records from the results of a query made elsewhere, and then to dispose of the results of the query.

Doesn't the first story read a lot better than the second? Aren't the concerns better separated and defined by the first one than the second? Just my humble $.02 worth.

To further stress my point, I'd say that if you were going to leave it the way it is, you'd want to rename mapResultsToRecord to mapResultsToRecordAndCloseResultSet.

UPDATE: @JohnKugelman's great idea provides yet another reason that in findRecords is the place to close the ResultSet. Only in here does it make sense to use a try-with-resource block, to get this alternative:

public List<Record> findRecords(String name) throws SQLException {
    String query = prepareQuery(name);
    try (ResultSet resultSet = executeQuery(query)) {
        return mapResultToRecord(resultSet);
    }
}

This version is guaranteed to close the ResultSet, even if mapResultToRecord throws an exception. Note the throws SQLException on the method signature. This is necessary because, even though you don't see the <ResultSet>.close() call, it's there implicitly, and it can throw a SQLException.

Upvotes: 2

Related Questions