Handle an exception from another function

Question

I would like to generate a method which opens a SQL-connection:

public SqlConnection openConnection (string connString)
{
    using (SqlConnection conn = new SqlConnection(connString))
    {
        try
        {
            conn.Open();
        }
        catch (Exception)
        {
           OrdersFaultException connEx = new OrdersDBInternalFaultException();
           throw new FaultException<OrdersFaultException>(connEx);
           return null;
        }
        return conn;
    }
}

I am wondering if the above code is a correct way? Or should I just throw an exception in the openConnection and treat the exception in the function that calls openConnection?

Answer 1

You can't use using here: no-one wants a disposed connection. So you could emulate using:

public SqlConnection openConnection (string connString) {
  SqlConnection conn = null;

  try {
    conn = new SqlConnection(connString));

    return conn;
  }
  catch (Exception e) { // <- Probably, you should catch more specific exception, e.g. DataException
    // This "if" (that's a part of typical using emulation scheme) is redundant
    // you may safely remove it unless there's no addition code in the "try"
    // see Todd Bowles comments
    if (!Object.ReferenceEquals(null, conn))
      conn.Dispose();

    // Do not forget to pass the reason (intitial exception e)
    // when throwing your own one
    throw new OrdersDBInternalFaultException(e);    
  }
}

Answer 2

You should handle exceptions of the same level of abstraction as the code raising these exceptions.

That means, code responsible for opening a database may throw a database related exception and a code responsible for handling orders may throw an order related exception.

You may want to throw something like a DatabaseConnectionException (with appropriate error message) from your openConnection method, and then raise your OrdersDBInternalFaultException from somewhere where you take care of your Orders in case DatabaseConnectionException occurred.

Side note: you don't need to return null after throwing an exception. That return won't ever be reached and is a dead code.

Answer 3

Both Dmitry and Tim are correct.

For exception handling though In your case as you are trying to open a connection therefore if the desired functionality is not achieved then exception should be thrown. So, that user knows that the functionality that function had to perform hasn't been performed.

Answer 4

Back in the days of C++, exception safety was really a thing of importance. If I remember correctly, Exceptional C++ by Sutter described this in quite a bit of detail.

One thing that might strike people as odd nowadays is that there is no 'finally' in C++. If you add create an object on the stack, it will be 'disposed' automatically when you exit the scope. To put it bluntly, scope is the thing that '{' and '}' will manage for you.

If you're doing C#, exception handling is still important, but on a different level. In all cases, you have to ensure that your overall state remains consistent. So how does this work?

using is nothing more than a wrapper around try, catch and finally. It basically creates Dispose in your finally, thereby ensuring that when you exit the scope (for whatever reason), the object is cleaned up (that is, assuming it implements IDisposable). Because of this, the easiest way to ensure objects are cleaned up properly, is by:

1. Creating an instance,
2. Using the instance,
3. Disposing the instance

(in this order!)

E.g.:

using (var connection = new Connection())  // create instance
{ 
    // use connection instance here

} // dispose connection instance -> implicitly created by 'using'.

Re-using code

If you don't want to write the code all over again, which is a good thing (f.ex. with the connection strings), you either manually dispose the objects or you can use a simple callback function to keep the flow the same.

private void Execute(Action<Connection> action) 
{
    using (var connection = new Connection())  // create instance
    { 
        // use connection instance here

        action(connection); // callback; this won't break the described mechanism

    } // dispose connection instance -> implicitly created by 'using'.
}

Why not using using properly is bad

So here's an implementation that returns the connection without any consideration for using:

// Buggy connection builder
private Connection CreateConnection()  
{ 
    Connection tmp = new Connection();
    // do some stuff with tmp, might throw

    return tmp;
}

private void Client() 
{
    using (var connection = CreateConnection())
    {
        // do something with the connection, assuming we're safe
    }
}

This seems all nice and okay, but it is actually incorrect. Can you see?

In the part where it said "do some stuff with tmp", something might go wrong, which causes the method to throw. When this happens, the object is never returned to the Client, leaving an alive Connection without a reference. Even though you assumed you were safe, you're actually not.

About constructors and exceptions

If you've paid attention, this also means that things can go wrong in constructors. If you create an object there that isn't cleaned up properly if shit hit the fan, you also have a dangling object. For example:

// Buggy connection wrapper
public class ConnectionWrapper : IDisposable
{ 
    public ConnectionWrapper() 
    {
        this.connection = new Connection();
        // do some other stuff, some of which might throw
    }

    private Connection connection;
    // ...

    // IDisposable stuff.
}

private void Client() 
{
    using (var connection = new ConnectionWrapper())
    {
        // do something with the connection, assuming we're safe
    }
}

Again, the code in the constructor can throw an exception. In this case the constructor will not return an object to the client, which means IDisposable won't be called in the finally of the using. This doesn't mean IDisposable is wrong here-- in fact it isn't!

What goes wrong here is that we create an IDisposable object that isn't returned and isn't cleaned up. We can fix this by adding a catch that simply re-throws the exception after cleaning up the connection.