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.

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?

share|improve this question
    
In the 'getVersion()' method, are you sure you want the return-value to be "" when there is a connection error? –  rolfl May 23 at 13:52
    
@rolfl Yes because I do this and I know if something is wrong: if (!version.trim().isEmpty()) System.out.println(version);. So I can just add an else in there if I want to handle it. –  Sillicon Touch May 23 at 13:54

2 Answers 2

up vote 20 down vote accepted

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:

enter image description here

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;
    }
}
share|improve this answer
    
So in my examples should I have two return statements, one in try and one in catch? What if there are multiple exceptions? Should every single one of them have a return? It's just that the code is repeated quite a lot that way. –  Sillicon Touch May 23 at 13:48
    
@SilliconTouch - updated my answer with a re-worked implementation of getVersion. –  rolfl May 23 at 13:58
    
I will check it out in about an hour due to an errand I have and I will get back to you. –  Sillicon Touch May 23 at 14:02
    
I would ask why you are returning anything at all when your database call has failed. That's a pretty major failure, and you should not be swallowing it. –  Mr Cochese May 23 at 14:31
    
@DevFromAbove - I had the same question (see the comment on the main question). –  rolfl May 23 at 14:51

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.

share|improve this answer
    
Thank you for your contribution I have one question though, why is it better to have a try-catch statement as minimal as possible? In my case for example why would it be better to have 3 try-catch when I could just have one where all the resources would be defined? –  Sillicon Touch May 30 at 11:26
    
Added a detailed explanation to my answer. –  janos May 30 at 13:20
    
Thank you very much I will research the two libraries that you mentioned as well. –  Sillicon Touch May 30 at 13:29

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.