Programmatic check or try...catch

Question

I have some code here:

public static void OpenConnection(IDbConnection connection)
{
    if(connection == null)
        throw new ArgumentNullException("connection", "The connection was null.");

    if (connection.State != ConnectionState.Closed)
        connection.Close();
}

The code has to be executed quite a lot since I open and close the connection every time I do something in the database. I wonder if the next code would be a better solution performance wise:

public static void OpenConnection(IDbConnection connection)
{
    try
    {
        connection.Close();
    }
    catch (NullReferenceException nullReferenceException) { throw; }
    catch (Exception exception) { } // This will occur if the connection was already closed so nothing should be done then.
}

PS. Is the catch (Exception exception) { } necessary?

EDIT: Replaced ArgumentNullException by NullReferenceException in the second code since that will be the exception when the connection == null.

Answer 1

I wonder if the next code would be a better solution performance wise

Consider what the performance and functional difference is in each case:

1. connection is null

You will get a NullReferenceException instead of an ArgumentNullException, which is a functional difference since you get a different exception type (and less context on why/where the exception occurs). If you decide to catch the NullReferecenException and throw an ArgumentNullException, thne you have the overhead of throwing a new exception, so there's a performance hit.

2. The connection is not closed.

An attempt to close the connection is made - no real performance or functional difference here.

3. The connection is closed

You try to close the connection again. Probably not a huge functional difference here (since most providers probably don't get mad if you try to close a connection that's already closed), but it's unnecessary and may have some performance disadvantages depending on what the Close() method actually does.

So your second method has functional differences and may actually have a disadvantage performance wise.

Stick to the code that illustrates the expected behavior more cleanly - then only optimize if you have a measurable, correctable performance issue.

Answer 2

Apart from JDB's argument of exceptions being costly, take a close look at your code and tell me which of those is much easier to read/follow?

If you've never seen a method and it starts with a "try" you really need to think and take a close look. If it however starts with your guard clause (the if (connection == null) part), which by the way is a very common thing to do, you will see immediately without even having to think that if you pass null into the method you will get an exception. Take this guard clause as a contract. You never want null to be passed in there. It is much better design.

About the 'PS'. If you were to do this, remember that ALL other exceptions that might be thrown in connection.Close() will be caught and, unless done by you, never surface. Such things might get your application to incur bugs that are very hard to track down.

Answer 3

According to Microsoft, Exceptions are a huge performance hit. Try to avoid them whenever reasonable:

Throwing exceptions can be very expensive, so make sure that you don't throw a lot of them. Use Perfmon to see how many exceptions your application is throwing. It may surprise you to find that certain areas of your application throw more exceptions than you expected. For better granularity, you can also check the exception number programmatically by using Performance Counters.

Finding and designing away exception-heavy code can result in a decent perf win. Bear in mind that this has nothing to do with try/catch blocks: you only incur the cost when the actual exception is thrown. You can use as many try/catch blocks as you want. Using exceptions gratuitously is where you lose performance. For example, you should stay away from things like using exceptions for control flow.

https://msdn.microsoft.com/en-us/library/ms973839.aspx#dotnetperftips_topic2

Your second example is actually a worse scenario. You should hardly ever catch general exceptions. You may think you know what will be thrown, but it's very possible that something unexpected will be thrown instead, leading to system instability and possible data loss/corruption.

It's Still Wrong to Use Catch (Exception e)

Even though the CLR exception system marks the worst exceptions as CSE, it's still not a good idea to write catch (Exception e) in your code. Exceptions represent a whole spectrum of unexpected situations. The CLR can detect the worst exceptions—SEH exceptions that indicate a possibly corrupt process state. But other unexpected conditions can still be harmful if they are ignored or dealt with generically.

In the absence of process corruption, the CLR offers some pretty strong guarantees about program correctness and memory safety. When executing a program written in safe Microsoft Intermediate Language (MSIL) code you can be certain that all the instructions in your program will execute correctly. But doing what the program instructions say to do is often different from doing what the programmer wants. A program that is completely correct according to the CLR can corrupt persisted state, such as program files written to a disk.

https://msdn.microsoft.com/en-us/magazine/dd419661.aspx

Answer 4

Your Second solution is not better for the performance, because your application will work harder when try block cause the exception, the catch block will try to catch exception. But the second solution is much better on logical point.

On your first solution you will get an error on your first check when connection is null.

Try-Catch or Try-Catch-Finally are powerful tools to handle errors, but they are expensive. Check out this link to see what you can do with it: Using Try... Catch..., Finally!

For better performance I would use:

private static void OpenSqlConnection(string connectionString)
{
using (SqlConnection connection = new SqlConnection(connectionString))
{
    connection.Open();
    //Do some work
}
}

The above example creates a SqlConnection, opens it, does some work. The connection is automatically closed at the end of the using block.

For your code for Try-catch I would do (to catch exception):

try
{
conn.Close();
}
catch (InvalidOperationException ex)
{
Console.WriteLine(ex.GetType().FullName);
Console.WriteLine(ex.Message);
//for Asp.net app
//Response.Write(ex.GetType().FullName);
// Response.Write(ex.Message);
}

For try catch please see: this link - Best Practices for Exceptions