null check in try-with-resources

0
votes

I've got the following code:

try (Connection connection = getConnection();

    PreparedStatement preparedStatement = connection.prepareStatement(someSql)) {//stuff}

How do I check that connection is not null here?

Also, I got a method which returns a PreparedStatement like this:

private PreparedStatement getPreparedStatement(Connection connection)

  throws SQLException {

PreparedStatement preparedStatement = connection.prepareStatement(SQL);

preparedStatement.setString(1, "someString");

return preparedStatement;

}

this gets called within a method that uses the following try with resources:

try (Connection connection = getConnection();
    PreparedStatement preparedStatement =
        getPreparedStatement(connection)) {//stuff}

Now I would assume the prepared statement will be autoclosed, because it gets initiated in the try with resources. But SonarCloud says that i should use try with resources in the getPreparedStatement-method or close that PreparedStatement in a finally block. Is this a wrong finding by SonarCloud or is there a better way to do it?

2
javatry-catchtry-with-resourcesnull-checksonarcloud
So is this two separate questions? As in the first block of code is a different question from the rest? – Nexevis
Definitely a wrong finding by SonarCloud. And it’s far from the first. Code analysis tools are hard to write. – VGR

2 Answers

1
votes

getConnection should throw an exception instead of returning null. Returning null is not nice. SonarCloud seems to be wanting you to put a try-with-resource opening ingetPreparedStatement(must include thesetString`) and closing in the calling method, which of course you can't do as such.

The best approach is the Execute Around idiom. Instead of getPreparedStatement returning a PreparedStatement pass in a lambda (typically) to be executed. The resource can then be closed in a try-with-resource statement cleanly.

/*** NICE ***/
// Function instead of Consumer would allow the method to return a value.
private void prepared(
    Connection connection, Consumer<PreparedStatement> op
) throws SQLException {
    // Might want to add getConnection in here too, perhaps.
    try (
        PreparedStatement statement =
             connection.prepareStatement(SQL)
    ) { 
        statement.setString(1, "someString");
        op.accept(statement);
    }
}

Used as:

    try (Connection connection = getConnection()) {
        prepared(connection, statement -> {
            // blah, blah, blah
        });
    }

The hacky alternative is to include a try statement within getPreparedStatement that only closed in error conditions.

/*** HACKY ***/
private PreparedStatement prepared(
    Connection connection
) throws SQLException {
    boolean success = false;
    PreparedStatement statement =
        connection.prepareStatement(SQL);
    try { 
        statement.setString(1, "someString");
        success = true;
        return preparedStatement;
    } finally {
        if (!success) {
            statement.close();
        }
    }
}

If you desperately want getConnection to return null (don't) and use a single two-entry try-with-resource, then the conditional operator works.

try (
    Connection connection = getConnection();
    PreparedStatement statement =
        connection==null ? null : connection.prepareStatement(SQL)
) {
0
votes

Easiest approch and IMO the most readable one would be to use 2 try-with-resources blocks. Even sonar has an built-in exception from disallowing nested try-catch-blocks for exactly this use case...

try (Connection conn = getConnection()) {
    if (conn != null) {
        try (PreparedStatement stmt = ...) {
            // do stuff
        }
    }
}

If do stuff is lengthy, it could and should be refactored into a separate method (possibly including the inner try-with-resources).