Return to Answer

• 1

• 2k
• 11
• 20

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.