Code smell - LINQ to SQL Datacontext usage and more… - Look from a bird's eye view

Alright! Lets have a look at this nice piece of code that I posted before.

As we follow the "Boy Scout rule" we try to improve this code every now and then. That means we get to a point where we are happy today but we need to accept that we are not getting it perfect today…

First of all, let us try to understand the responsibility of this class and the intention behind it.

1. The name is "EmployeePositionAssignment" and it uses a data context and has 2 constants. That is not too bad, but doesn't really tell us what the intention is.
2. The class-wide defined datacontext suggest that we are reusing that datacontext multiple times which is a little worry so far, since MSDN is suggesting us to avoid this. From http://msdn.microsoft.com/en-us/library/system.data.linq.datacontext.aspx

The DataContext is the source of all entities mapped over a database connection. It tracks changes that you made to all retrieved entities and maintains an "identity cache" that guarantees that entities retrieved more than one time are represented by using the same object instance.

In general, a DataContext instance is designed to last for one "unit of work" however your application defines that term. A DataContext is lightweight and is not expensive to create. A typical LINQ to SQL application creates DataContext instances at method scope or as a member of short-lived classes that represent a logical set of related database operations.

 

    public class EmployeePositionAssignment
    {
        private NorthWindDataContext db = new NorthWindDataContext();
        private static readonly TimeSpan defaultTimeSpan = new TimeSpan(1, 0, 0, 0);
        private static readonly DateTime defaultMaxDate = new DateTime(2100, 1, 1);
      // -- snip snip snip --

 

The 1st bit of worrying code is the constructor that opens a database connection, although we are using LINQ2SQL as an ORM mapper here.

        public EmployeePositionAssignment()
        {
            db.Connection.Open();
        }

Since we are using an ORM framework we shouldn't need to do this and let the framework manage the connection for us.
That raises a question: When are we going to close the connection?

 

The next few lines show us the answer to the previous question.

        //~EmployeePositionAssignment()
        //{
        //    if (db.Connection.State == ConnectionState.Open)
        //    {
        //        db.Connection.Close();
        //    }
        //    db.Dispose();
        //}

Someone tried to close the database connection in the finalizer of the class.
I am not saying "destructor" here, since we C# developers should think about this method as a finalizer and not destructor in a C++ sense. More on this topic and the differences can be found on MSDN and on Christian Nagel's blog entry about C++/CLI, Finalize and Dispose
There are a couple of problems with this finalizer:

• Finalizers should *only* be used to clean up unmanaged resources.
  We should very rarely need a finalizer these days actually
• It would be better to implement "IDisposable" and let the allow users of the class when the connection is not needed anymore

Heaps of more great information and guidelines can be found as part of the Framework Design Guidelines. There are some nice patterns on how to implement IDisposable too.

Back to the initial question: Where is connection closed then? Probably as part of the GC removing up the type "NorthWindDataContext", LINQ2SQL will close the connection, but not before. Which means multiple instances of this class can use up all connections to the database and we are presented with a YSOD, pretty soon

 

The next piece of evil code in this is the following method: "ApplyDepotChangesToShifts"

1. We are passing the whole "NorthWindDataContext" to the method, which is clearly violating the "Tell don't ask" principle
   This comes close to: "Hey I give you everything and you just take what you need --> Out of control"
   
2. We have a parameter "bool isDelete" which suggests us that this method is doing 2 things. We should avoid boolean parameters and have 2 separate methods instead

private static int ApplyDepotChangesToShifts(
                           NorthWindDataContext dataContext, 
                           EmployeeDepotHistoryAssignment assignment, 
                           bool isDelete, 
                           DateTime originalDateBegin)
        {

 

The next worrying discovery (with the help of Resharper) is that all methods are "public" although some could be "private". By making some methods "private" in this 400 line helper class we could help the next reader to understand the intention of the class and help applying some refactorings to separate responsibilities.

 

Conclusion (so far)

Just by having a bird's eye view on this class we spotted already a couple of issues we need to fix.
First of we should probably start by removing the class-wide DataContext and making sure that every method uses its own datacontext properly.

 

How did we get here?

I am trying to understand the current situation and how we got into this mess. This might help me avoid this situation in the future…
#1 There were no Code Reviews in place, otherwise another developer would have pointed out: "Hey, I don't get this bit here" or maybe just yelling out: "WTF" as illustrated in this nice comic: "The only valid measurement of code quality"

#2 Automated tools could help here as well: NDepend, StyleCop, VS2010 Code Analysis or FxCop (What is the difference?) could definitely help to tell us the most worrying code in this solution

 

Problems ahead

Before we touch any code, we should ensure that all tests are green around this specific piece of code.
2 problems here:

1. There are no tests around this code , which means I am on my own when I am changing something.
   There is no way for me to find out if I broke something or not, unless manual testing through the UI. As we know manual testing is cumbersome, costs time and is NOT FUN.
2. This class uses a the "NorthWindDataContext" type which is instantiated as a field in the class itself. There is no Dependency injection in place where I could inject my own mock of that class, which leads me to write some Integration tests for this class  

Now I am off to actually refactor this piece of code with my favorite tools: Visual Studio 2010 and ReSharper. I spotted more improvements on the method level. Stay tuned!