1

I am updating some code to allow us to implement unit tests. What I have so far is a Business Layer which calls the repository to get data from the database.

Business Layer example:

public class ConversationLogic
{
    private IConversationData _conversationData { get; set; }

    public ConversationLogic(IConversationData conversationData)
    {
        _conversationData = conversationData;
    }

    public ConversationListModel GetConversations(int loginID)
    {
        ConversationListModel model = new ConversationListModel();

        dynamic list = _conversationData.GetConversations(loginID);

        foreach (var item in list)
        {
            ConversationModel conversation = ConvertConversation(item);

            model.Conversations.Add(conversation);
        }

        return model;
    }
}

This allows an Interface of ConversationData to be passed in, this ConversationData is the repository layer which calls the database.

Conversation Data repository example:

public class ConversationData : BaseMassiveTable, IConversationData
{
    public dynamic GetConversations(int loginID)
    {
        string sql = GetResourceFile("Project.SQL.GetConversations.sql", Assembly.GetExecutingAssembly());
        dynamic result = Query(sql, loginID).ToList();
        return result;
    }
}

The ConversationData inherits from the class BaseMassive, which implements IDisposable:

public class BaseMassiveTable : DynamicModel, IDisposable
{
    string connectionName;

    public BaseMassiveTable(string connectionStringName, string tableName, string primaryKeyColumn) :
        base(connectionStringName, tableName, primaryKeyColumn) 
    { 
        connectionName = connectionStringName;
    }

    public void Dispose()
    {
    }

    protected void WriteErrorToFile(int userId, string url, Exception ex)
    {
        try
        {
            string path = "~/Error/" + DateTime.Today.ToString("yyyy-MM-dd") + ".txt";
            if (!File.Exists(System.Web.HttpContext.Current.Server.MapPath(path)))
            {
                File.Create(System.Web.HttpContext.Current.Server.MapPath(path)).Close();
            }
            using (StreamWriter w = File.AppendText(System.Web.HttpContext.Current.Server.MapPath(path)))
            {
                w.WriteLine("\r\nLog Entry : ");
                w.WriteLine("{0}", DateTime.Now.ToString(CultureInfo.InvariantCulture));
                string err = "Error in: " + url +
                              ". Error Message:" + ex.Message;
                w.WriteLine(err);
                w.WriteLine(ex.StackTrace.ToString());
                w.WriteLine("__________________________");
                w.Flush();
                w.Close();
            }
        }
        catch
        {
            //What do we do here ????
        }
    }

    public string Truncate( string stringForTuncation, int maxLength)
    {
        string result = "";
        if (stringForTuncation == null)
        {
            result = null;
        }
        else if (stringForTuncation.Length <= maxLength)
        {
            result = stringForTuncation;
        }
        else
        {
            result = stringForTuncation.Substring(0, maxLength);
        }
        return result;
    }

    public static string GetResourceFile(string name, Assembly assembly)
    {
        using (Stream stream = assembly.GetManifestResourceStream(name))
        {
            using (StreamReader reader = new StreamReader(stream))
            {
                string result = reader.ReadToEnd();
                return result;
            }
        }
    }

}

As you may seen in my implementation of ConversationLogic I cannot wrap the IConversationData interface in a using as this does not work with the current implementation.

What you will also see that in ConversationData.GetConversations(), this calls the Massive ORM Query(), which looks like this:

public virtual IEnumerable<dynamic> Query(string sql, params object[] args)
{
    using (var conn = OpenConnection())
    {
        var rdr = CreateCommand(sql, conn, args).ExecuteReader();
        while (rdr.Read())
        {
            yield return rdr.RecordToExpando(); ;
        }
    }
}

What I am looking for is that, even though ConversationData inherits from BaseMassive, which implements IDisposable, do I actually need to wrap any calls using IConversationData in using statements, as every call to the repository calls the Massive Query() statement, which manages its own resources by creating a using() statement on any query?

Apologies for the long-winded question, if you need any clarifications, just let me know.

3
  • Yes. And this means a ConversationLogic instance should only exist inside the using block where ConversationData exists. Otherwise you get resource leaks.
    – JacquesB
    Jun 28, 2017 at 14:13
  • Remove IDisposable and Dispose. It has no purpose. Jun 28, 2017 at 22:58
  • I don't really understand the question. ConversationLogic isn't instantiating anything disposable. Jul 7, 2017 at 15:23

3 Answers 3

3

The caller should use using; you need to implement Dispose in order for him to be able to. Then the Dispose calls will cascade and everything will get released in due course.

The class into which the disposable resource is injected should have its own Dispose method and should release the resources there. Then you need to make sure that whoever is using the ConversationLogic class is also disposing it, perhaps with a using clause of their own.

For example, if your ConversationLogic class is injected into an MVC controller, you can take advantage of the fact that the base Controller class already has a Dispose method. You just need to override it to Dispose your ConversationLogic, which will in turn release your resource-intensive service. The .NET pipeline will call the controller's Dispose method at the right time.

class MyController : System.Web.Mvc.Controller
{
    private readonly IConversationLogic _conversationLogic;

    public MyController(IConversationLogic conversationLogic)
    { 
        _conversationLogic = conversationLogic;
    }

    public override void Dispose(bool disposing)
    {
        if (disposing)
        {
            if (_conversationLogic != null)
            {
                _conversationLogic.Dispose(true);
                _conversationLogic = null;
            }
            base.Dispose(true);
        }
    }
}

class ConversationLogic : IConversationLogic, IDisposable
{
    private readonly _conversationData;

    public ConversationLogic(IConversationData conversationData)
    {
        _conversationData = conversationData;
    }

    public override Dispose(bool disposing)
    {
        if (disposing)
        {
            if (_conversationData != null)
            {
                _conversationData.Dispose();
                _conversationData = null;
            }
        }
        base.Dispose(disposing);
    }
}

Sometimes you're not sure if the injected resource implements IDisposable. It is easy to check.

    public override Dispose(bool disposing)
    {
        if (disposing)
        {
            if (_conversationData != null)
            {
                var d = _conversationData as IDisposable;
                if (d != null) 
                {
                    d.Dispose();
                }
                _conversationData = null;
            }
        }
        base.Dispose(disposing);
    }

If this pattern bothers you (because you're a purist, and you noticed that the code that created the resource didn't dispose it) you have two options:

  1. Get over it, because the code that created the resource was the IoC container, and it would require something really weird for the IoC container to deal with the disposal, or

  2. Don't inject the disposable instance; inject a factory instead. That way your class can both create and dispose of the object and make everyone happy.

    class ConversationLogic: IConversationLogic, IDisposable
    {
        private readonly _conversationData;
    
        public ConversationLogic(IConversationDataFactory conversationDataFactory)
        {
            _conversationData = conversationDataFactory.GetInstance();
        }
    
        public override Dispose(bool disposing)
        {
            if (disposing)
            {
                if (_conversationData != null)
                {
                    _conversationData.Dispose();
                    _conversationData = null;
                }
            }
            base.Dispose(disposing);
        }
    }
    

Either way, you don't use using, you use the Dispose override per the conventional pattern, and ensure that your controller (or other object) is itself enclosed in a using clause so that Dispose gets called.

2

Your current design violates the Interface segregation principle which states that

no client should be forced to depend on methods it does not use.

so either the BaseMassiveTable should not require the IDisposable interface or the ConversationData should not be derived from this type because it looks like it's not the right base type for it. This means that you might even need one more level of abstraction that both the BaseMassiveTable and the ConversationData can be derived from but only the BaseMassiveTable would add the IDisposable interface because it introduces some disposable resources.

This way you would be able to correctly use a disposable type. Otherwise you need to know the ConversationData's implementation in order to know that it actually does not need to be disposed. This behavior is inconsistent and someone else will definitely wonder what is going on here and if he doesn't know that, he might actually try to fix this bug and dispose it properly. It is not ok to now dispose a disposable type for whatever reason. Even if it doesn't use unmanaged resources now, it might in future so if you now ignore this interface you might have memory leaks later.

1

Yes. Without seeing what BaseMassiveTable is, one can only assume it holds either native resources or other IDisposable resources. It's incumbent upon you to release those resources as soon as can be done, preferably via the using statement. You could, in this case, possibly exhaust database connection pool resources.

3
  • I have edited my question to include the full code of BaseMassiveTable. This class does not seem to do anything except set the ConnectionString, which is used within the Query() within the repository. Jun 28, 2017 at 14:14
  • Well, that's a little bit of a head-scratcher then. It doesn't look like that class (or your descendant) actually has any resources that need to be Disposed. At all. So, maybe my advice doesn't exactly apply to this situation? Odd design choice. Jun 28, 2017 at 14:23
  • Very odd, this is code that somebody else has implemented and I am trying to make testable, but it looks like that class does not need to implement IDisposable as they do not do anything with the Dispose(). Jun 28, 2017 at 14:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.