Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Is this an acceptable way to make sure a java.sql.ResultSet is always closed, and also make sure that an Exception caught is propagated to the caller?

Please don't hesitate to review other aspects of this sample as well.

public void updateReplicationStatus(BeanReplicationTask taskBean, String strStatus) throws Exception {
    String strSelect = this.createReplicationSelectStatement(taskBean);
    ResultSet resultSet = null;
    try {
        resultSet = DerbyDog.getResultSet(strSelect);
        if (resultSet.next()) {
            resultSet.updateString(COLUMN_STATUS, strStatus);
            resultSet.updateRow();
        }
    } catch (Exception e) {
        throw e;
    } finally {
        if (resultSet != null) {
            resultSet.close();
        }
    }
}
share|improve this question
    
To answer the questions: 2 times yes. –  tb- Dec 17 '12 at 14:15

2 Answers 2

Close a ResultSet/Resource

If you are using java 7 then I would recommend to use try with resources

it will take care of closing ResultSet for you and make your code a little cleaner.

public void updateReplicationStatus(BeanReplicationTask taskBean, String strStatus) throws Exception {
    String strSelect = this.createReplicationSelectStatement(taskBean);       
    try (ResultSet resultSet = DerbyDog.getResultSet(strSelect) {            
        if (resultSet.next()) {
            resultSet.updateString(COLUMN_STATUS, strStatus);
            resultSet.updateRow();
        }
    } catch (Exception e) {
        throw e;
    } 
}

Exception Handling

personal I prefer to wrap my own exceptions around generic exceptions.

That way I can throw a more precise exception with a comment to help myself when debugging

public void updateReplicationStatus(BeanReplicationTask taskBean, String strStatus)   throws ReplicationStatusException {
    String strSelect = this.createReplicationSelectStatement(taskBean);       
    try (ResultSet resultSet = DerbyDog.getResultSet(strSelect) {            
        if (resultSet.next()) {
            resultSet.updateString(COLUMN_STATUS, strStatus);
            resultSet.updateRow();
        }
    } catch (SqlException e) {
        throw new ReplicationStatusException("update ReplicationStatus failed because of a SQLException", e);
    } catch (Exception e) {
        throw new ReplicationStatusException("update ReplicationStatus failed because of an unexpected exception", e)
    }   
}

that's about my 2 cents

share|improve this answer
  1. +1 to Alex and an improvement for Java 6: you could eliminate the null check in the finally block if you move the DerbyDog.getResultSet call before the try block. If it's inside the try block and it throws an exception you won't be able to close the ResultSet because the reference will remain null. Guideline 1-2: Release resources in all cases in the Secure Coding Guidelines for the Java Programming Language, Version 4.0 documentation could be helpful.

  2. The catch block is unnecessary here. Exceptions will be propagated without it.

  3. Be aware of static helper classes. DerbyDog seems one of them. You might find my former answer about it useful.

ResultSet resultSet = DerbyDog.getResultSet(strSelect);
try {
    if (resultSet.next()) {
        resultSet.updateString(COLUMN_STATUS, strStatus);
        resultSet.updateRow();
    }
} finally {
        resultSet.close();
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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