Using finally with return statement or not

Question

I was wondering if it is the proper way to always put the return statement of a function with a try-catch clause in the finally clause.

I have for example this function which returns the version of an sql server:

public String getVersion() {
        String version = new String();
        final String query = "Select VERSION()";
        Connection con = null;
        Statement stmt = null;

        try {
            Helper.println("Getting driver...");
            Class.forName(_driver);

            Helper.println("Connecting to database...");
            con = DriverManager.getConnection(_url, _user._username, _user._password);
            stmt = con.createStatement();

            Helper.println("Executing query...");
            ResultSet rs = stmt.executeQuery(query);
            rs.next();
            version = rs.getString(1);
        } catch (SQLException se) {
            Helper.printErrln("SQL Error: " + se.getErrorCode() + ' ' + se.getMessage());
        } catch (ClassNotFoundException e) {
            Helper.printErrln("Driver Error: " + e.getMessage());
        } finally {
            try {
                if (stmt != null) {
                    stmt.close();
                }
            } catch (SQLException se) {
            }
            try {
                if (con != null) {
                    con.close();
                }
            } catch (SQLException se) {
            }  
            return version;
        }
    }

I return the version variable always in the finally clause in situations like these and it particularly convenient when I need to return a list because if something went wrong then I will just be left with an empty list which I prefer. For example:

public List<String> getDatabases() {
        List<String> databases = new ArrayList<>();
        Connection con = null;
        Statement stmt = null;

        try {
            Helper.println("Getting driver...");
            Class.forName(_driver);

            Helper.println("Connecting to database...");
            con = DriverManager.getConnection(_url, _user._username, _user._password);
            stmt = con.createStatement();

            Helper.println("Finding Databases...");
            ResultSet rs = con.getMetaData().getCatalogs();

            while (rs.next()) {
                databases.add(rs.getString("TABLE_CAT"));
            }
        } catch (SQLException se) {
            Helper.printErrln("SQL Error: " + se.getErrorCode() + " " + se.getMessage());
        } catch (ClassNotFoundException e) {
            Helper.printErrln("Driver Error: " + e.getMessage());
        } finally {
            try {
                if (stmt != null) {
                    stmt.close();
                }
            } catch (SQLException se) {
            }// do nothing
            try {
                if (con != null) {
                    con.close();
                }
            } catch (SQLException se) {
            }      
            return databases;
        }
    }

So first of all my question is: Is what I am doing proper or can it cause problems that I have not anticipated? Also what if I wanted to encapsulate the function and have it trow the exception to the parent class for handling, how would the return be handled then?

Answer 1

Return statement in the finally block is almost always a very bad idea.....

The sematics of the finally block are complicated, but, if there is a return statement the try block, or in a catch block, then those will be called, and then the finally block will run, and it will change the return value.

IDE's, and the compiler, will complain about this:

Code like the following, will return 2 .... always:

private static int doSomething() {
    try {
        .... something here....
        return 0;
    } catch (Exception e) {
        return 1;
    } finally {
        return 2;
    }

}

The point is that it is very bad practice to have returns in the finally block. Don't.....

EDIT: Update to include a better solution.... Use Java try-with-resources (since Java7).

private static final String VERSION_QUERY = "Select VERSION()";
private static final String VERSION_NOT_FOUND = "";

public String getVersion() {

    try {
        Helper.println("Getting driver...");
        Class.forName(_driver);
    } catch (ClassNotFoundException e) {
        Helper.printErrln("Driver Error: " + e.getMessage());
        return VERSION_NOT_FOUND;
    }


    Helper.println("Connecting to database...");
    try (Connection con = DriverManager.getConnection(_url, _user._username, _user._password);
         Statement stmt = con.createStatement();) {

        Helper.println("Executing query...");
        try (ResultSet rs = stmt.executeQuery(VERSION_QUERY);) {
            rs.next();
            return rs.getString(1);
        }
    } catch (SQLException se) {
        Helper.printErrln("SQL Error: " + se.getErrorCode() + ' ' + se.getMessage());
        return VERSION_NOT_FOUND;
    }
}

Answer 2

A few more tidbits to add on top of @rolfl's answer.

When getting the version, you didn't check the result of rs.next(), but I think you should. Sure, "normally" there will always be a record for the query SELECT VERSION(), but in case things are "not normal", your method will not blow up in your face with an Exception but gracefully return VERSION_NOT_FOUND.

You did not close the ResultSet anywhere. You should close it first, before Statement and Connection.

Try to include in a try-catch block only the minimum code that needs to be there. So I would rewrite the driver loading part like this:

Helper.println("Getting driver...");
try {
    Class.forName(_driver);
} catch (ClassNotFoundException e) {
    Helper.printErrln("Driver Error: " + e.getMessage());
    return VERSION_NOT_FOUND;
}

If you have Java 7+, then by all means use @rolfl's solution. If you are stuck with Java 6, you can use this:

public String getVersion() {
    Helper.println("Getting driver...");
    try {
        Class.forName(_driver);
    } catch (ClassNotFoundException e) {
        Helper.printErrln("Driver Error: " + e.getMessage());
        return VERSION_NOT_FOUND;
    }

    Connection con = null;
    Statement stmt = null;
    ResultSet rs = null;

    try {
        Helper.println("Connecting to database...");
        con = DriverManager.getConnection(_url, _user._username, _user._password);
        stmt = con.createStatement();

        Helper.println("Executing query...");
        rs = stmt.executeQuery(VERSION_QUERY);
        if (rs.next()) {
            return rs.getString(1);
        }
    } catch (SQLException se) {
        Helper.printErrln("SQL Error: " + se.getErrorCode() + ' ' + se.getMessage());
    } finally {
        try { if (rs != null) rs.close(); } catch (SQLException e) {}
        try { if (stmt != null) stmt.close(); } catch (SQLException e) {}
        try { if (con != null) con.close(); } catch (SQLException e) {}
    }
    return VERSION_NOT_FOUND;
}

Why limit a try block to minimum code

When a try block has minimal code, then it's perfectly clear where the exception came from. At the minimum, this often makes a difference in how you clean up gracefully. For example if you could not get a Connection, then you can return immediately, no need to worry about closing ResultSet and Statement.

In the worst case, a bug might be masked by a too broad try block. Imagine a try-catch where you call another method inside the try. You don't expect that other method to throw an exception, and if it does, that will be a bug. But if that exception gets handled by your too broad catch, then you will not notice the bug. That's why it's good to minimize the try block.

However, as far as database connection management goes, the cleanup path for Connection-Statement-ResultSet is such a PITA that I didn't split them in the example code above. This technique seems widely accepted, but it's definitely not perfect. In practice, I never work with Connection-Statement-ResultSet directly, I use Spring's JdbcTemplate, which greatly simplifies this pain. Another alternative is Apache's commons-db. I strongly recommend to use these or similar frameworks instead of struggling with the low-level Connection-Statement-ResultSet trio.