Htydjtk

This code is part of an application that reads from and writes to an ODBC connected database. It creates a record in the database and then checks if a record has been successfully created, then returning true.

My understanding of control flow is as follows:

command.ExecuteNonQuery() is documented to throw an Invalid​Operation​Exception when "a method call is invalid for the object's current state". Therefore, if that would happen, execution of the try block would stop, the finally block would be executed, then would execute the return false; at the bottom.

However, my IDE claims that the return false; is unreachable code. And it seems to be true, I can remove it and it compiles without any complaints. However, for me it looks as if there would be no return value for the code path where the mentioned exception is thrown.

private static bool createRecord(String table, 

                                 IDictionary<String,String> data, 

                                 System.Data.IDbConnection conn, 

                                 OdbcTransaction trans) {



    [... some other code ...]



    int returnValue = 0;

    try {

        command.CommandText = sb.ToString();

        returnValue = command.ExecuteNonQuery();



        return returnValue == 1;

    } finally {

        command.Dispose();

    }



    return false;

}

What is my error of understanding here?

The finally may run on an Exception, though that aside it doesn't change the fact (in this case) it will still be an Uncaught Exception. Ergo, the last return will never get hit regardless.

If you want the code to continue onto the last return, Catch the Exception; If you don't, just leave it the way it is and remove the return

Example

try 

{

    command.CommandText = sb.ToString();

    returnValue = command.ExecuteNonQuery();



    return returnValue == 1;

}

catch(<some exception>)

{

   // do something

}

finally 

{

    command.Dispose();

}

To be more clear from the documentation

try-finally (C# Reference)

By using a finally block, you can clean up any resources that are
allocated in a try block, and you can run code even if an exception
occurs in the try block. Typically, the statements of a finally block
run when control leaves a try statement. The transfer of control can
occur as a result of normal execution, of execution of a break,
continue, goto, or return statement, or of propagation of an exception
out of the try statement.

Within a handled exception, the associated finally block is guaranteed
to be run. However, if the exception is unhandled, execution of the
finally block is dependent on how the exception unwind operation is
triggered. That, in turn, is dependent on how your computer is set up.

Usually, when an unhandled exception ends an application, whether or
not the finally block is run is not important. However, if you have
statements in a finally block that must be run even in that situation,
one solution is to add a catch block to the try-finally statement.
Alternatively, you can catch the exception that might be thrown in the
try block of a try-finally statement higher up the call stack. That
is, you can catch the exception in the method that calls the method
that contains the try-finally statement, or in the method that calls
that method, or in any method in the call stack. If the exception is
not caught, execution of the finally block depends on whether the
operating system chooses to trigger an exception unwind operation.

the finally block would be executed, then would execute the return false; at the bottom.

Wrong. finally doesn't swallow the exception. It honors it and the exception will be thrown as normal. It will only execute the code in the finally before the block ends (with or without an exception).

If you want the exception to be swallowed, you should use a catch block with no throw in it.

The warning is because you didn't use catch and your method is basically written like this:

bool SomeMethod()

{

    return true;

    return false; // CS0162 Unreachable code detected

}

Since you use finally solely to dispose, the preffered solution is to utilize using pattern:

using(var command = new WhateverCommand())

{

     ...

}

That's enough, to ensure what Dispose will be called. It's guaranteed to be called either after successful execution of code block or upon (before) some catch down in call stack (parent calls are down, right?).

If it wouldn't be about disposing, then

try { ...; return true; } // only one return

finally { ... }

is enough, since you will never have to return false at the end of method (there is no need for that line). Your method is either return result of command execution (true or false) or will throw an exception otherwise.

Consider also to throw own exceptions by wrapping expected exceptions (check out InvalidOperationException constructor):

try { ... }

catch(SomeExpectedException e)

{

    throw new SomeBetterExceptionWithExplanaition("...", e);

}

This is typically used to say something more meaningful (useful) to the caller than nested call exception would be telling.

Most of times you don't really care about unhandled exceptions. Sometimes you need to ensure that finally is called even if exception is unhandled. In this case you simply catch it yourself and re-throw (see this answer):

try { ... }

catch { ...; throw; } // re-throw

finally { ... }

It seems, you are looking for something like this:

private static bool createRecord(string table, 

                                 IDictionary<String,String> data, 

                                 System.Data.IDbConnection conn, 

                                 OdbcTransaction trans) {

  [... some other code ...]



  // using: do not call Dispose() explicitly, but wrap IDisposable into using

  using (var command = ...) {

    try {

      // Normal flow:

      command.CommandText = sb.ToString();



      // true if and only if exactly one record affected

      return command.ExecuteNonQuery() == 1; 

    }

    catch (DbException) {

      // Exceptional flow (all database exceptions)

      return false;

    }

  }

}   

please, note, that finally doesn't swallow any exception

finally {

  // this code will be executed; the exception will be efficently re-thrown

}



// and this code will never be reached

When the exception is thrown, the stack will unwind (execution will move out of the function) without returning a value, and any catch block in the stack frames above the function will catch the exception instead.

Hence, return false will never execute.

Try manually throwing an exception to understand the control flow:

try {

    command.CommandText = sb.ToString();

    returnValue = command.ExecuteNonQuery();



    // Try this.

    throw new Exception("See where this goes.");



    return returnValue == 1;

} finally {

    command.Dispose();

}