Method to re-open a connection on a datacontext if it has gone down

Question

The following method is used to keep a DataContext open if it is closed or broken and is called before most calls to access the database.

It runs in a service that needs constant access to the database, constantly opening and closing the connection would be too costly. So we only reconnect if the connection has been closed, i.e. we have been kicked off.

private void OpenConnection<T>(System.Data.Linq.DataContext dataContext, string connectionString) where T : System.Data.Linq.DataContext
{
 try
 {
 if (dataContext != null && dataContext.Connection != null)
 {
 if (dataContext.Connection.State == System.Data.ConnectionState.Closed
 || dataContext.Connection.State == System.Data.ConnectionState.Broken)
 {
 dataContext.Dispose();
 dataContext = null;
 }
 }
 }
 catch
 {
 if (dataContext != null)
 {
 dataContext.Dispose();
 }
 dataContext = null;
 }
 if (dataContext == null)
 {
 dataContext = (T)System.Activator.CreateInstance(typeof(T), connectionString);
 }
}

It is called like:

OpenConnection<MyDataContext>(instanceOfMyDataContext, connectionString);

My main concerns are if my use of generics is correct and if the as T on creating an instance of the DataContext is correct. Although all pointers about my code are welcomed.

• Adriano Repetti: If you create an instance of base class then cast will always fail. Use Activator.CreateInstance() instead
• TheLethalCoder: @AdrianoRepetti I copied this from an old version of the method not realising that line was still in there, as no one as answered yet I have updated the question to show the use of Activator. If this is frowned upon I will revert the edit though
• Adriano Repetti: Another small thing: dataContext parameter should be ref, like this you're writing a new instance on local variable and caller (then also future calls) won't see the new instance. (I'm not sure but I suppose it's OK to update a question until there isn't an answer)
• TheLethalCoder: @AdrianoRepetti I feel that's more of a review than a comment if you want to expand it into an answer? Either way thanks
• Adriano Repetti: Too small (IMO) for an answer. I have a question: isn't connection pooling effective in your case? There shouldn't be any serious performance hit with a normal approach (with using) until connection is kept in pool and reused for each call (also I think EF caches something on its own to reduce construction overhead if you do not pass a connection string and you read it from .config)

Answer 1

First off, not sure I fully understand why you're doing this but taking that as a given here are some tips to make your code a bit more readable and concise.

Namespaces

Unless you have conflicts with other namespaces, I'd suggest adding using System.Data; and using System.Data.Linq; to the top of you file. If you do have a conflict and you created it yourself (not from a 3rd-party library) I'd suggest changing the name so there isn't a conflict if possible. Just makes things easier.

Method Extensions

Those if-statements you've got are kind of ugly. You could create a couple of method extensions for the DataContext class like this.

public static class DataContextExtensions
{
 /// <summary>
 /// Checks that the context and its connection are not null.
 /// </summary>
 public static bool HasContextAndConnection(this DataContext dataContext)
 {
 return dataContext != null
 && dataContext.Connection != null;
 }
 /// <summary>
 /// Checks if the context's connection is broken or closed.
 /// </summary>
 public static bool IsConnectionClosedOrBroken(this DataContext dataContext)
 {
 // NOTE: You can add the above null-checks to this one so that this 
 // function doesn't throws a NullReferenceException if called before the other.
 return dataContext.Connection.State == ConnectionState.Closed
 || dataContext.Connection.State == ConnectionState.Broken;
 }
}

Try-Catch

You should be intentional with your code, having a wide-open try/catch states to whoever is reading it "The person who coded this has no idea what could possibly go wrong with it." (or more commonly, "I'm too lazy to list out what could go wrong."). So not only does it make you look bad, but it also doesn't provide any information to the reader/maintainer of what is expected to go wrong.

So then you should look into what could go wrong. Well, we're already doing null-checks so there shouldn't be any NullReferenceExceptions. That leaves the Dispose() method and the Activator.CreateInstance being the only other two possibilities.

According to the IDisposable.Dispose() MSDN page, Dispose should never throw an exception. Additional StackOverflow reference here too. So we should be good there.

Activator.CreateInstance however, could throw a handful of exceptions and you don't have a try-catch around that. Here's the MSDN page that outlines the exceptions that could be thrown.

Result

private void OpenConnection<T>(DataContext dataContext, string connectionString) 
 where T : DataContext
{
 // DISPOSE: Clean up the existing context if the connection is closed or broken.
 if (dataContext.HasContextAndConnection() && !dataContext.IsConnectionClosedOrBroken())
 {
 dataContext.Dispose();
 dataContext = null;
 }
 // CREATE: Instantiate a new context given the connection string.
 if (dataContext == null)
 {
 var type = typeof(T);
 try
 {
 dataContext = Activator.CreateInstance(type, connectionString);
 }
 catch (TargetInvocationException)
 {
 // Constructor threw an exception.
 }
 }
}

Personal Touch

Personally, I like adding the header comments you see above. Where the all-caps part is a quick 1-2 word description of the section and everything after is a bit more detailed. I find it helps me scim code faster, however that is something that needs maintained when code is changed.

• t3chb0t: I'm not happy with the comment part. If you would use proper helper variables then you wouldn't need the comments. The first one could be canDisposeContext and the second one contextExists. The code would explain itself. Your comments explain what you do but I can already read it looking at the code. You only should and need to explain why you are doing something. This cannot usualy be easily discovered.
• Shelby115: A technical paper's paragraphs explain the paper and yet they still have headers as a quick description. I agree that code should explain itself, and your comment should explain why if-not inherently obvious. I just find them as a nice quick skim to find a section of code, especially if you go weeks without touching the code again. Maybe even in an ideal world you wouldn't need such headers, but in my experience the ideal world is just that, an Idea.