views:

79

answers:

2

Hello,

At work, I've found a helper class to manage WCF Services which implements IDisposable and has a ServiceAgent that derives from System.ServiceModel.ClientBase. The Dispose() method closes all the opened WCF services. The helper exposes methods that wraps calls to the methods of the ServiceAgent. Each method is build on that pattern:

public void WCFMethod1()
{
    using(this)
    {
        this.ServiceAgent.WCFMethod1();
    }
}

public override void Dispose()
{
    try
    {
        this.ServiceAgent.Close();
    }
    catch
    {
        this.ServiceAgent.Abort();
    }
    finally
    {
        this.ServiceAgent = null;
    }
}

Here's the question: is the use of using(this) a good practice?

A: 

I don't like it. I think that class should be a) disposed once b) by code that instantiated it. IDisposable usually understood as way to implement RAII pattern. MSDN explicitly states:

If an object's Dispose method is called more than once, the object must ignore all calls after the first one. The object must not throw an exception if its Dispose method is called multiple times. Instance methods other than Dispose can throw an ObjectDisposedException when resources are already disposed.

So users will find this behavior confusing, so i recommend to design the class so that it is instantiated per call:

using (var clientFactory = new ClientFactory())
   clientFactory.Client.WCFMethod1();
Andrey
A: 

For sure this code is a bad practice. It is the caller who decides whether the instance is needed or not. It is supposed, that after the Dispose method is called, no more method calls are allowed (according to guidelines, the instance switches to disposed state). Imagine you call some method, then another one and get ObjectDisposed exception. Kind of weird, isn't it?

Pavel Fedulov